Generating a musical scale from a root note

Question

I'm looking to improve my coding skills. I am new to programming, and after weeks of research and trial and error - I have successfully wrote this basic program to find a music scale by inputting a Root note and Scale.

Since I am pretty much teaching my self - I am looking for criticism to improve my syntax/programming. I compressed the code listed below and localized variables into a 46 line version of this program but really like the layout of the original, so please be brutally honest. Pros and Cons with explanations please?

Note: I was purely being lazy by using #define and plan to replace all strings and vectors with their correct std::typename.

#include <iostream>
#include <string>
#include <vector>
#include <map>
#define astring std::string
#define cvector std::vector
astring rootnote = "";
astring scale = "";
void notation();
int root;
cvector<astring> notes;
cvector<astring> order;
void scaler();
int main()
{
 while (rootnote != "null") {
 std::cout << "Please enter your root note and scale: " << std::endl;
 std::cin >> rootnote >> scale;
 std::cout << "\nroot scale: " << rootnote << " " << scale << std::endl;
 std::cout << std::endl;
 notation();
 scaler();
 }
 return 0;
}
void notation()
{
 notes.clear();
 cvector<astring> chromatic = { "C", "Db", "D", "Eb", "E", "F", "Gb", "G", "Ab", "A", "Bb", "B" };
 root = std::distance(chromatic.begin(), std::find(chromatic.begin(), chromatic.end(), rootnote));
 for (int i = root; i < chromatic.size(); i++) { notes.push_back(chromatic[i]); }
 for (int j = 0; j < root; j++) { notes.push_back(chromatic[j]); }
 return;
}
void scaler() {
 order.clear();
 std::map<astring, cvector<int>> scales;
 scales["Major"] = { 0, 2, 4, 5, 7, 9, 11 };
 scales["Minor"] = { 0, 2, 3, 5, 7, 8, 10 };
 scales["Melodic"] = { 0, 2, 3, 5, 7, 9, 11 };
 scales["Harmonic"] = { 0, 2, 3, 5, 7, 8, 11 };
 for (auto i : scales[scale])
 order.push_back(notes[i]);
 for (int j = 0; j < order.size(); j++)
 std::cout << order[j] << " ";
 std::cout << "\n\n" << std::endl;
 return;
}

• Richard Christopher: I haven't included the if statement for sharps yet. You have to use 'Gb Major'

Answer 1

Yikes — the global variables make this spaghetti code. Functions should make it obvious what their input parameters are, and return their output values. You'd be better off lumping everything into one main() function than using global variables. Ironically, the two variables that should have been global constants — chromatic and scales — weren't.

It is customary to put main() last, and helper functions first, so that you don't have to write forward declarations.

You used std::find() and std::distance(), so you should #include <algorithms> and <iterator>. I suggest listing your includes alphabetically.

Behaviour

Please enter your root note and scale: 
F# Major
root scale: F# Major
C D E F G A B 

If the rootnote is not found in chromatic, you should handle that case instead of blindly proceeding based on chromatic.end().

Please enter your root note and scale: 
D Major
root scale: D Major
D E Gb G A B Db 

Those enharmonic equivalents will sound the same, but those should technically be F♯ and C♯.

Please enter your root note and scale: 
null blah
root scale: null blah

Making the user enter the magic word "null" as the root note to exit the program is weird.

Suggested solution

#include <algorithm>
#include <iostream>
#include <iterator>
#include <map>
#include <sstream>
#include <string>
#include <vector>
const std::vector<std::string> chromatic = {
 "C", "Db", "D", "Eb", "E", "F", "Gb", "G", "Ab", "A", "Bb", "B"
};
const std::map<const std::string, const std::vector<int>> degrees = {
 {"Major", { 0, 2, 4, 5, 7, 9, 11, 12 }},
 {"Minor", { 0, 2, 3, 5, 7, 8, 10, 12 }},
 {"Harmonic", { 0, 2, 3, 5, 7, 8, 11, 12 }},
 {"Melodic", { 0, 2, 3, 5, 7, 9, 11, 12 }}
};
std::string scalenotes(const std::string& rootnote, const std::string& scale) {
 int root = std::distance(chromatic.begin(), std::find(chromatic.begin(), chromatic.end(), rootnote));
 if (root >= chromatic.size()) {
 return "";
 }
 std::stringstream ss;
 for (int i : degrees.at(scale)) {
 ss << chromatic[(root + i) % chromatic.size()] << " ";
 }
 return ss.str();
}
std::stringstream ask(const std::string& question) {
 std::string line;
 std::cout << question;
 std::getline(std::cin, line);
 return std::stringstream(line);
}
int main()
{
 std::string rootnote, scale;
 while (ask("Please enter your root note and scale: ") >> rootnote >> scale) {
 std::cout << "\nroot scale: " << rootnote << " " << scale
 << ": " << scalenotes(rootnote, scale) << "\n\n";
 }
}

• Richard Christopher: I initially had a really hard time following your suggestion, but after reading it a few times .. I greatly appreciate you taking the time to go through my code and offer a solution with a thorough explanation. I've kind of been stuck due to the (lack of) format in my original design. I'll definitely be taking your advice. I'll update you guys when I clean up all the loose ends.
• alephzero: The data for "pentatonic" is wrong. That is a whole-tone scale, not a pentatonic scale. (Hint: "pentatonic" means "5 notes", not 6). The name "minor" isn't very good either - but that's more a issue about music theory than programming!
• Richard Christopher: I'll be adding more scales and more functionality after I incorporate the advice provided above. I do agree about the naming and intervals of minor @alephzero but that's another topic for another place..
• Richard Christopher: I scratched the entire idea but wanted to give a big thanks to 200_success for the beautiful revision and introducing me to some new (to me) concepts
• Richard Christopher: I have referred back to your suggested code multiple times 200_success.. it's a beautiful solution. I just had to say. Your answer has been really helpful and enlightening. I will be sure to share the updated solution when I get to that level.

Answer 2

1. You should avoid the global vector variable and instead pass it by reference into the functions. I would remove all global variables and pass them through functions instead.

2. The input is a little weird. You could do it differently using getline. It should not be checking if you type "null." Also what if you just pass in one string, it would blow up I'm pretty sure. Something like this:

   string line;
   while (getline(std::cin, line))
   {
    // line contains entire line user entered
   }
   

3. Keep your for loop format and brackets consistent.

4. There is no reason to define different names for vector and string either, just confuses other developers. If you do not want to type std::vector, std::cout everytime you use it, you can declare at the top "using std::cout"

Other than that it seems pretty straightforward. Good job :D

The main thing that is frowned upon is the global variables, not passing references through functions, and how you are getting user input.

• Richard Christopher: 1. I rewrote the program eliminating all external functions and internalized the variables. I haven't thought about using ref&.. I'll definitely try that. 2. How would suggest taking the input to complete the functions? 3. I didn't even notice. Thanks for pointing that out. 4. You're right. I was being lazy. .. 5. is there a way to post updated code without removing the original for comparison?
• Graipher: @RichardChristopher Re 5.: You can always ask a new question with the updated code. If you link back to this question with the note that it is a follow-up question, you might even get away with being a bit more succinct in the description of the problem statement.
• Nick Gallimore: You could update the code here as well. I do not mind. Or provide a link to the new question. I would use the getline function. I updated my answer with some code.
• 200_success: @nfgallimore The rules of this site prohibit updating the code in the question or adding revised code after answers have been posted.
• Nick Gallimore: Yup you can split it based on " "