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

            Is the Concept of Multiple Fantasy Races Scientifically Flawed? [closed]

            Image
            Clash Royale CLAN TAG #URR8PPP up vote 2 down vote favorite Many fantasy worlds have multiple humanoid species (elves, orcs, humans, etc) living in the same world, in addition to that, they often have the ability to interbreed. And I can't help but question their believability... fantasy-races reproduction interspecies-relations share | improve this question edited Aug 9 at 14:46 Michael Kjörling ♦ 21.1k 10 85 161 asked Aug 9 at 13:13 Chlodio 118 6 closed as unclear what you're asking by MichaelK, Gryphon, John, Vincent, Frostfyre Aug 9 at 15:25 Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question. 3 Stack Exchange exist to
            Read more

            Confectionery

            Image
            Prepared foods made with a lot of sugar or other cabohydrates "Sweetmeat" redirects here. For the racehorse, see Sweetmeat (horse). Not to be confused with Sweetbread. This Kransekake is a traditional Scandinavian baker's confection. Confectionery is the art of making confections , which are food items that are rich in sugar and carbohydrates. Exact definitions are difficult. [1] In general, though, confectionery is divided into two broad and somewhat overlapping categories, bakers' confections and sugar confections. [2] Bakers' confectionery, also called flour confections , includes principally sweet pastries, cakes, and similar baked goods. Sugar confectionery includes candies ( sweets in British English), candied nuts, chocolates, chewing gum, bubble gum, pastillage, and other confections that are made primarily of sugar. In some cases, chocolate confections (confections made of chocolate) are treated as a separate category, as are sugar-free versions of
            Read more