Generating a musical scale from a root note

The name of the pictureThe name of the pictureThe name of the pictureClash Royale CLAN TAG#URR8PPP











up vote
4
down vote

favorite












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;






c++ beginner music







share|improve this question









New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.















  • 1




    For input F# Major, I get C D E F G A B as the output?
    – 200_success
    2 hours ago










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










  • You should avoid the global vector variable and instead pass it by reference into the functions. Also the input is a little weird. You could do it differently.
    – nfgallimore
    1 hour ago














up vote
4
down vote

favorite












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;






c++ beginner music







share|improve this question









New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.















  • 1




    For input F# Major, I get C D E F G A B as the output?
    – 200_success
    2 hours ago










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










  • You should avoid the global vector variable and instead pass it by reference into the functions. Also the input is a little weird. You could do it differently.
    – nfgallimore
    1 hour ago












up vote
4
down vote

favorite









up vote
4
down vote

favorite











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;






c++ beginner music







share|improve this question









New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











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;






c++ beginner music




c++ beginner music






share|improve this question









New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.











share|improve this question









New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









share|improve this question




share|improve this question








edited 2 hours ago









200_success

125k14144402




125k14144402






New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.









asked 2 hours ago









Richard Christopher

211




211




New contributor




Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.





New contributor





Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.






Richard Christopher is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.







  • 1




    For input F# Major, I get C D E F G A B as the output?
    – 200_success
    2 hours ago










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










  • You should avoid the global vector variable and instead pass it by reference into the functions. Also the input is a little weird. You could do it differently.
    – nfgallimore
    1 hour ago












  • 1




    For input F# Major, I get C D E F G A B as the output?
    – 200_success
    2 hours ago










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










  • You should avoid the global vector variable and instead pass it by reference into the functions. Also the input is a little weird. You could do it differently.
    – nfgallimore
    1 hour ago







1




1




For input F# Major, I get C D E F G A B as the output?
– 200_success
2 hours ago




For input F# Major, I get C D E F G A B as the output?
– 200_success
2 hours ago












I haven't included the if statement for sharps yet. You have to use 'Gb Major'
– Richard Christopher
2 hours ago




I haven't included the if statement for sharps yet. You have to use 'Gb Major'
– Richard Christopher
2 hours ago












You should avoid the global vector variable and instead pass it by reference into the functions. Also the input is a little weird. You could do it differently.
– nfgallimore
1 hour ago




You should avoid the global vector variable and instead pass it by reference into the functions. Also the input is a little weird. You could do it differently.
– nfgallimore
1 hour ago










2 Answers
2






active

oldest

votes

















up vote
1
down vote













  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.


  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.






share|improve this answer






















  • 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?
    – Richard Christopher
    1 hour ago

















up vote
1
down vote













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 ,
 "Pentatonic", 0, 2, 4, 6, 8, 10, 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";






share|improve this answer






















    Your Answer




    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: false,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );






    Richard Christopher is a new contributor. Be nice, and check out our Code of Conduct.









     

    draft saved


    draft discarded


















    StackExchange.ready(
    function ()
    StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f204472%2fgenerating-a-musical-scale-from-a-root-note%23new-answer', 'question_page');

    );

    Post as a guest

















    2 Answers
    2






    active

    oldest

    votes








    2 Answers
    2






    active

    oldest

    votes









    active

    oldest

    votes






    active

    oldest

    votes








    up vote
    1
    down vote













    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.


    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.






    share|improve this answer






















    • 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?
      – Richard Christopher
      1 hour ago














    up vote
    1
    down vote













    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.


    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.






    share|improve this answer






















    • 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?
      – Richard Christopher
      1 hour ago












    up vote
    1
    down vote










    up vote
    1
    down vote









    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.


    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.






    share|improve this answer














    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.


    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.







    share|improve this answer














    share|improve this answer



    share|improve this answer








    edited 1 hour ago

























    answered 1 hour ago









    nfgallimore

    995




    995











    • 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?
      – Richard Christopher
      1 hour ago
















    • 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?
      – Richard Christopher
      1 hour ago















    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?
    – Richard Christopher
    1 hour ago




    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?
    – Richard Christopher
    1 hour ago












    up vote
    1
    down vote













    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 ,
     "Pentatonic", 0, 2, 4, 6, 8, 10, 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";






    share|improve this answer


























      up vote
      1
      down vote













      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 ,
       "Pentatonic", 0, 2, 4, 6, 8, 10, 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";






      share|improve this answer
























        up vote
        1
        down vote










        up vote
        1
        down vote









        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 ,
         "Pentatonic", 0, 2, 4, 6, 8, 10, 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";






        share|improve this answer














        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 ,
         "Pentatonic", 0, 2, 4, 6, 8, 10, 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";







        share|improve this answer














        share|improve this answer



        share|improve this answer








        edited 9 mins ago

























        answered 46 mins ago









        200_success

        125k14144402




        125k14144402




















            Richard Christopher is a new contributor. Be nice, and check out our Code of Conduct.









             

            draft saved


            draft discarded


















            Richard Christopher is a new contributor. Be nice, and check out our Code of Conduct.












            Richard Christopher is a new contributor. Be nice, and check out our Code of Conduct.











            Richard Christopher is a new contributor. Be nice, and check out our Code of Conduct.













             


            draft saved


            draft discarded














            StackExchange.ready(
            function ()
            StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f204472%2fgenerating-a-musical-scale-from-a-root-note%23new-answer', 'question_page');

            );

            Post as a guest
































































            Comments

            Post a Comment

            Popular posts from this blog

            Long meetings (6-7 hours a day): Being “babysat” by supervisor

            Image
            Clash Royale CLAN TAG #URR8PPP .everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0; up vote 31 down vote favorite 4 I hold a PhD in mechanical engineering with a 7 year background in self-guided, self-paced research including 2 years as a graduate level course instructor and a subject matter expert for various engineering projects in a university setting in the United States. For the last 10-11 months, I am working in a research industry in France where my task is to debug C++ spaghetti code written over a period of several years by my current supervisor. It is a team of 2: just I and my supervisor. Diurnally, I am posed with a "bug of the day" to get out of this C++ code. My supervisor generally gives me about 15-30 minutes to solve it and then accosts me about whether or not the task is completed and then rinses and repeats this until the end of the day. Off-late, my supervisor has been holding 6-7 hour long "meetin
            Read more

            What does second last employer means? [closed]

            Image
            Clash Royale CLAN TAG #URR8PPP .everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0; up vote -3 down vote favorite Say, for example I have worked in the following companies, Company A => 2010 - 2011 Company B => 2011 - 2012 Company C => 2012 - Till Date Company D => Have offer Now assume Company D is asking to submit documents from my second last employer. So from the list of companies given, which company would be qualified as second last employer? software-industry employer share | improve this question asked Aug 10 '14 at 5:16 Rajaprabhu Aravindasamy 559 1 6 13 closed as off-topic by Jim G., jmort253 ♦ Aug 11 '14 at 1:58 This question appears to be off-topic. The users who voted to close gave this specific reason: "Questions seeking advice on company-specific regulations, agreements, or policies should be directed to your manager or HR department. Questions that address only
            Read more

            One-line joke

            A one-liner is a joke that is delivered in a single line. A good one-liner is said to be pithy - concise and meaningful. [1] Comedians and actors use this comedic method as part of their act, e.g. Jimmy Carr, Tommy Cooper, Rodney Dangerfield, Norm Macdonald, Ken Dodd, Stewart Francis, Zach Galifianakis, Mitch Hedberg, Anthony Jeselnik, Milton Jones, Shappi Khorsandi, Jay London, Mark Linn-Baker, Demetri Martin, Groucho Marx, Dan Mintz, Emo Philips, Tim Vine, Steven Wright and Henny Youngman and Arnold Schwarzenegger. [ citation needed ] Many fictional characters are also known to deliver one-liners, including James Bond, who usually includes pithy and laconic quips after disposing of a villain. [ citation needed ] Examples "A baby seal walks into a club." "A dyslexic man walks into a bra." (Anonymous) "There are three types of people, those who can count and those who can't." "Venison's dear, isn't it?" (Jimmy Carr) "An escala
            Read more