mjhjmtu

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 << "nn" << std::endl;

 return;

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) << "nn";
 

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.