Program that outputs the chords in a given key

Question

I am built a program that will give the 7 chords that are found a given major or minor key. My program takes in first a key (C, Ab, F#, etc.) then takes in its mode (Major (input is 'M') and minor (denoted by 'm')). From there the program builds the chords in that key in a specific manner depending on the mode where you take a pattern of whole (W) and half (H) steps.

For major keys, the pattern is to move (from the original position): WWHWWW. The 2nd, and 3rd chords in the key are minor and the 6th is diminished (denoted by '°'). The rest are major chords.

Minor keys is similar to: WHWWHW. The 1st, 4th, and 5th are minor chords, and the 2nd is a diminished. The rest are major.

My code build the chords in a weird way. I have a string array with the name of the chords in them called sChords. When I get a specific key input, a call my function called GetScale which orchestrates the whole program. It returns a string which I add to another string in the main function. The first thing GetScale is it adds the first chord which is very easy.

It then calls FindKey which returns an integer (into keyInt) such that the integer corresponds to the index of that key in the sChords array. I then use a function in a for loop called IndexNumberForChordArray where I send the key index, the mode, and the current iteration in the for loop. Over there the steps are calculated.

[The steps are done such that for a whole step, the pointer for sChords moves 2 over, and for a half, only one over.]

After that, AdditionsToChord is called which adds the 'm' for minor chords and '°' for diminished ones.

I have a couple of weird quirks in my program.

First is the useless bool variable in GetScale called flatted. I have that there because when my program builds a key, it often repeats letters for weird key signatures like C#. The general rule of thumb in music theory is that when describing a scale, each letter should appear only once. I have that there so that I can later make it so that the program can print only one of each letter. How? Well, I am not too sure if it will work, but I was thinking something like:

If the key is flatted, unless a chord is a natural (i.e. no sharps or flats), it should also be flatted. Otherwise always use natural or sharped chords.

The other problem that might be evident is that mode is an integer when it can easily be a bool. I have it as an integer because I hope to expand this program to include more than the major and minor scales. There are 7 scales in western music and I hope to add that into this program.

I am uncomfortable with the current method however, and want to see how it can be improved. I especially don't like that fact that I have to use a string array and move a pointer around the array to reach a specific chord. I felt like there might be an easier mathematical way, maybe using ASCII, but I didn't know how to do it because of all the sharps and flats.

Another thing is that I don't like is that I am using a large switch structure to identify the index of a given key. Maybe this can be improved too.

One last thing that is a bad habit for me is using goto. I know it is bad practice, but I thought it might not matter in this specific situation.

Some useful links are:

1. http://www.piano-keyboard-guide.com/chords-by-key.html

2. https://en.wikipedia.org/wiki/Mode_(music)

// I am planning on adding different modes + making the scales have one of every letter.
using System;
namespace ChordFinder
{
 class Program
 {
 //This will probably be removed for a more dynamic way of creating the scale.
 static string[] sChords = { "Ab", "A", "Bb", "B", "C", "C#", "D", "Eb", "E", "F", "F#", "G" };
 static void Main(string[] args)
 {
 string key, mode;
 int[] nChords = new int[7];
 //Get key
 Start:
 Console.Write("Enter key (type 'Exit' to exit): ");
 key = Console.ReadLine();
 //Check if input is exit
 if (key.ToUpper() == "EXIT" || key == null)
 Environment.Exit(1);
 else if (key[0] > 71 || key[0] < 65)
 {
 Console.WriteLine("Invalid key, try again.");
 goto Start;
 }
 //Get mode
 ModeStart:
 Console.Write("Enter mode ('M' for major, 'm' for minor): ");
 mode = Console.ReadLine();
 //Checks if input is 'M' or 'm'
 if (mode[0] != 77 && mode[0] != 109)
 {
 Console.WriteLine("Invalid mode, try again.");
 goto ModeStart;
 }
 string a = "";
 if (mode[0] == 77)
 a = GetScale(key, 0, false);
 else
 a = GetScale(key, 1, false);
 Console.WriteLine("The chords are: " + a);
 Console.WriteLine();
 goto Start;
 }
 //Gets the scale. Note: flatted not developed yet.
 static string GetScale(string key, int mode, bool flatted)
 {
 string a = string.Empty;
 int keyInt;
 //Set the first chord.
 a += key;
 if (mode == 1)
 a += "m";
 a += " ";
 //Set point to sChords array.
 keyInt = FindKey(key);
 for (int i = 1; i < 7; i++)
 a += sChords[IndexNumberForChordArray(ref keyInt, mode, i)] + AdditionsToChord(mode, i) + " ";
 return a;
 }
 //Returns an integer that corresponds to the
 //index of the sChords array.
 static int FindKey(string key)
 {
 switch (key)
 {
 case "G#":
 case "Ab":
 return 0;
 case "A":
 return 1;
 case "A#":
 case "Bb":
 return 2;
 case "B":
 return 3;
 case "C":
 return 4;
 case "C#":
 case "Db":
 return 5;
 case "D":
 return 6;
 case "D#":
 case "Eb":
 return 7;
 case "E":
 return 8;
 case "F":
 return 9;
 case "F#":
 case "Gb":
 return 10;
 default:
 return 11;
 }
 } 
 //Finds the index for the correct chord
 static int IndexNumberForChordArray(ref int start, int mode, int iteration)
 {
 if (mode == 0) 
 {
 //Major key algorithm
 start += 2;
 if (iteration == 3)
 start -= 1;
 start %= 12;
 }
 else if (mode == 1)
 {
 //Minor key algorithm
 start += 2;
 if (iteration == 2 || iteration == 5)
 start -= 1;
 start %= 12;
 }
 return start;
 }
 //Adds extra stuff depending on the mode and position in the scale.
 static string AdditionsToChord(int mode, int iteration)
 {
 string s = string.Empty;
 if (mode == 0)
 {
 //Major key algorithm
 if (iteration == 1 || iteration == 2 || iteration == 5)
 s = "m";
 if (iteration == 6)
 s = "°";
 }
 else if (mode == 1)
 {
 //Minor key algorithm
 if (iteration == 3 || iteration == 4)
 s += "m";
 if (iteration == 1)
 s += "°";
 }
 return s;
 }
 }
}
```

• BCdotWEB: You really shouldn't use (some kind of) Hungarian notation: joelonsoftware.com/2005/05/11/making-wrong-code-look-wrong .
• MrMineHeads: I don't understand, is my naming convention wrong? I guess keyInt and sChords falls into that, but is it bad?
• user73941: I think the steps for the major keys should be: WWHWWWH. And it's the seventh chord that is diminished - not the sixth (The sixth chord of C major is plain A minor (parallel key) and the seventh chord is B diminished (Bm7b5))
• MrMineHeads: @HenrikHansen the last half step brings you back to C just an octave higher/lower. If you include the dominant, you get 7 chords using what I wrote.
• user73941: Yes, but you include seven steps for the minor scale, so I just thought they should contain the same number of steps :-).

Answer 1

I dabble in music so your post caught my eye.

Here are my thoughts:

1. If I had to pick one word to describe the app, it would be "procedural". You have successfully defined an algorithm, but I'd say that it reads more like FORTRAN than C#. It even includes goto... which I've never seen used before in C#.

2. When working in an object-oriented language, I prefer to let the domain drive the object model. In my experience, modelling what exists in reality goes a long way toward making software maintainable and expandable.

3. Instead of a giant switch statement, you might want to go with a Dictionary<string, int>

In my spare time I whipped up my take on an object-oriented musical scale and chord "generator" app, which I have posted here: https://github.com/lucidobjects/MusicalKeys

In keeping with my point #2, there are a bunch of objects, and certainly a lot more lines of code. But, I feel the object model gives us the beginnings of a foundation upon which we could build a music theory app of arbitrary size - and remain sane in the process.

My app lacks some of the features that yours has, like interactive input and modes. But, my aim was to show an object-oriented approach to the problem rather than achieve feature parity.

Here’s a screenshot of the class files. While the preferred terminology may vary from musician to musician, various elements of music theory are present and accounted for.
Classes

To see how the program runs, check out App.cs.

public void Run()
{
 var steps = new Steps();
 var scalePatterns = getScalePatterns(steps).Select(p => p as Pattern).ToList();
 var intervals = new Intervals();
 var chordPatterns = getChordPatterns(intervals).Select(p => p as Pattern).ToList();
 var tones = new Pitches();
 ///define the Key of A and output individual elements from it
 var keyOfA = new Key(tones.A, scalePatterns, chordPatterns);
 Console.WriteLine(keyOfA.Scales.Get("Major").ToString());
 Console.WriteLine(keyOfA.Scales.Get("Natural Minor").ToString());
 Console.WriteLine(keyOfA.Scales.Get("Melodic Minor").ToString());
 Console.WriteLine(keyOfA.Chords.Get("M").ToString());
 Console.WriteLine(keyOfA.Chords.Get("M7").ToString());
 Console.WriteLine(keyOfA.Chords.Get("7").ToString());
 Console.WriteLine(keyOfA.Chords.Get("m7").ToString());
 Console.WriteLine(keyOfA.Chords.Get("m7b5").ToString());
 Console.WriteLine(keyOfA.Chords.Get("dim7").ToString());
 Console.WriteLine();
 ///shortened syntax to define the Key of F# and output all of its scales and chords
 Console.WriteLine(new Key(tones.Fsharp, scalePatterns, chordPatterns).ToString());
 ///define and print a single scale
 var scalePattern = new StepPattern("Major Scale", "WWHWWWH");
 var scale = new ToneSet(tones.Dsharp, scalePattern);
 Console.WriteLine(scale.ToString());
 ///define and print a single chord
 var chordPattern = new IntervalPattern("Major 7th", "M7", "P1 M3 P5 M7");
 var chord = new ToneSet(tones.C, chordPattern);
 Console.WriteLine(chord.ToString());
 ///define and print another single chord
 var chordPattern2 = new IntervalPattern("Dominant Seventh", "7", "P1 M3 P5 m7");
 var chord2 = new ToneSet(tones.G, chordPattern2);
 Console.WriteLine(chord2.ToString());
}

And here’s the output:
Output

• MrMineHeads: I very much appreciate your work. I understand that c# should primarily be used for its object orientation but I was unsure how to use objects. Thanks for the answer, I have a better idea of how to work with it.
• Aron: Thanks, glad to help. Object Oriented Programming is a big topic, which I enjoy studying. If you're going to be using C#, or any OOP language, I encourage you to learn more about OOP. You might find my other C# code review answers interesting...

Answer 2

Just looking at this one section:

if (key.ToUpper() == "EXIT" || key == null)
 Environment.Exit(1);
else if (key[0] > 71 || key[0] < 65)
{
 Console.WriteLine("Invalid key, try again.");
 goto Start;
}

• Why Environment.Exit(1) and not simply exit(1)?
• Given that the program has already exited for the true condition, there is no point in having the else.

• Where do "71" and "65" come from?
  Other than "0", "1", and "many", raw numbers should never appear in code.
  In this case they are actually the ASCII values for "A" and "G".
  So why not simply say 'A' and 'G'?

• "Invalid key, try again." is a very frustrating error message.
  Don't tell me that it is invalid; tell me why it is invalid.
  And why is "Burble" considered valid?

• There are times when goto is appropriate.
  But such times are few and far between, and definitely not in this program.

Similar comments apply to most of the rest of the program.

• MrMineHeads: I really don't know why I used 71 and 65, but I'll make that fix. The "invalid key" error was more of a placeholder, but I'll make sure to make it more specified. I also forgot that "Burble" shouldn't work. I'll set up a check for that. Thank you.

Answer 3

Its good you recognize that goto is a bad pattern, I would go so far I would reject any code that I reviewed that had it. They can be converted to a loop statement pretty easy.

For Example

var key = string.Empty;
//Get key
do
{
 Console.Write("Enter key (type 'Exit' to exit): ");
 key = Console.ReadLine();
 if (key != "EXIT" || key[0] > 'G' || key[0] < 'A')
 {
 Console.WriteLine("Invalid key, try again.");
 key = string.Empty;
 }
} while (string.IsNullOrWhiteSpace(key));
var mode = string.Empty; 
//Get mode
while (key != "EXIT" && string.IsNullOrWhiteSpace(mode))
{
 Console.Write("Enter mode ('M' for major, 'm' for minor): ");
 mode = Console.ReadLine();
 if (mode[0] != 'M' || mode[0] != 'm')
 {
 Console.WriteLine("Invalid mode, try again.");
 mode = string.Empty;
 }
}; 

No need for Environment.Exit and if wanted to keep user in it's just another loop that check for exit that wraps these two.

Instead of comparing the ASCII number you can use the real letter just using a single quote. See above. When looking at code latter someone will need to look up what ASCII 71,65,77,109 where if you replace it, like the example, it's clear what values the program is checking for.

The variable nChords isn't used. To go along with that is pretty standard in C# to declare your variable at time of use and not at the top of the program.

Lets also talk naming. What is a? It's not descriptive variable at all. Variables should have meaning. Could call it scale or even result would be better than a. Naming is one of the harder things in programming but someone or even you coming back later and looking at this code would not know what a means. Don't prefix your variables/properties/fields with their type. I'm assuming sChords starts with s because it's a string array. just name it cords without the prefix. Same would go with nChords but you can just remove that variable all together.

The GetScale call can be inlined

var scale = GetScale(key, mode[0] == 'M' ? 0 : 1, false);

Also with GetScale it doesn't use parameter flatted.

I'm also not a fan of ref parameters in general and would suggest to find a way to not be passing it in by reference.

• MrMineHeads: I addressed the flatted parameter in my post saying that it is for future development. Other than that, I hadn't noticed the nChords, that must have been early on in my project. I'll remove that. I also will switch to using characters instead of the ASCII counterparts.
• CharlesNRice: I usually prescribe to YAGNI and don't add stuff to programs until it's needed.