7
\$\begingroup\$

I've been working on a solution to Pset3 of CS50 Harvard course for last couple of hours and managed to finish it. Although it works as intended I'm unhappy with how clean this code is, mostly because of use of multiple nested conditionals inside the main switch case. The function takes a string, for example "B#3", and converts it to correct frequency.

If interested here is the Pset and here is the frequency table.

// Calculates frequency (in Hz) of a note
int frequency(string note)
{
    const double a4Frequency = 440;
    char key;
    float numOfSemitones = 0;
    double octave = 0;
    bool flat, sharp;

    // Determine octave and whether key is sharp or flat
    if (note[1] == 'b')
    {
        octave = note[2] - '0';
        flat = true;
    }
    else if(note[1] == '#')
    {
        octave = note[2] - '0';
        sharp = true;
    }
    else
        octave = note[1] - '0';

    key = note[0];

    // Base frequency knowing that A4 is 440 hz
    if(((key == 'A') && (octave == 4)) && ((!flat) && (!sharp)))
        return 440;

    // TODO find alternative way to count distance between keys - there is a pattern
    // Count frequency using distance in semitones from A4
    double frequency = 0.0;
    switch(key)
    {
        case 'B':
            if (flat)
                numOfSemitones = 1;
            else
                numOfSemitones = 2;

            frequency = pow(2, (numOfSemitones / 12))  * a4Frequency;
            break;
        case 'A':
            if (flat){
                numOfSemitones = 1;
                frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            }
            else if (sharp){
                numOfSemitones = 1;
                frequency = pow(2, (numOfSemitones / 12))  * a4Frequency;
            }
            else
                frequency = a4Frequency;

            break;
        case 'G':
            if (flat)
                numOfSemitones = 3;
            else if (sharp)
                numOfSemitones = 1;
            else
                numOfSemitones = 2;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'F':
            if (sharp)
                numOfSemitones = 3;
            else
                numOfSemitones = 4;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'E':
            if (flat)
                numOfSemitones = 6;
            else
                numOfSemitones = 5;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'D':
            if (flat)
                numOfSemitones = 8;
            else if (sharp)
                numOfSemitones = 6;
            else
                numOfSemitones = 7;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
        case 'C':
            if (sharp)
                numOfSemitones = 8;
            else
                numOfSemitones = 9;

            frequency = a4Frequency / pow(2, (numOfSemitones / 12));
            break;
    }

    // Multiply or divide frequency to get note in correct octave
    int final;
    if (octave > 4)
    {
        octave = pow(2, (octave - 4));
        final = round(frequency * octave);
    }
    else if (octave == 4)
    {
        octave = 0.0;
        final = round(frequency);
    }
    else
    {
        octave = pow(2, (4 - octave));
        final = round(frequency / octave);
    }
    return final;
}

I'm trying to focus on writing clean code from start, yet this solutions seems a bit hacked to me. How can I make this code more readable? What's the alternative to nested conditionals? What's "illegal" about this code?

\$\endgroup\$
  • 1
    \$\begingroup\$ @Peter please leave the indentation as is, unless it isn't formatted as code. Refer to this meta answer as to why and alternative actions. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Aug 10 '18 at 18:02
  • 1
    \$\begingroup\$ This is tagged as "C", but uses string which is either not defined here or is C++'s std::string. A little odd. \$\endgroup\$ – user1118321 Aug 11 '18 at 5:36
  • 1
    \$\begingroup\$ @user1118321 believe it or not, there's a dedicated SE site for this course: CS50 with an an answer explaining that it's a typedef in the header file cs50.h that looks like this: typedef char *string; \$\endgroup\$ – I'll add comments tomorrow Aug 11 '18 at 12:04
7
\$\begingroup\$

Overall this is pretty readable code. It's straightforward to understand, so it would be fairly easy to maintain or debug if there were found to be problems.

Table-driven Development

If you look at the various case statements within your switch you'll notice that they all take the same form, just with different values. This indicates that it might be a good idea to use a table of values to reduce the amount of code. I would build a table of the number of semitone change for each note like this:

int kSemitoneOffsets[] = { -2, 0, 2, 4, 5, 7, 9 };

In the natural case where there are no accidentals (sharps or flats) applied, every case from your switch statements breaks down to this:

frequency = a4Frequency / pow(2, (kSemitoneOffsets [ index ]  / 12));

Here index is calculated from the key character. If we don't mind rearranging the above table so it progresses from A up to G, it ends up looking like this:

int kSemitoneOffsets[] = { 0, -2, 9, 7, 5, 4, 2 };

Now you can calculate the index like this:

int index = key - 'A';

Next, we can handle accidentals by adding or subtracting 1 from the lookup. Putting it all together, it looks like this:

int kSemitoneOffsets[] = { 0, -2, 9, 7, 5, 4, 2 };
int index = key - 'A';
int numOfSemitones = kSemitoneOffsets [ index ];
if (flat)
{
    numOfSemitones--;
}
else if (sharp)
{
    numOfSemitones++;
}
frequency = a4Frequency / pow(2, (numOfSemitones / 12));

Notice that I changed the calculation for B from a multiplication to a division so it was the same form as the other calculations. This is possible because raising a number to a negative power is the same as dividing by that number raised to the positive power.

We can use this same trick to calculate final without the if statement, like this:

final = round(frequency * pow(2, octave - 4));

When octave is less than 4, we get a negative number and end up dividing. When it's 0, we get 1 so the multiplication does nothing.

Remove Unnecessary Code

In addition to switching to using a table, you can also remove unnecessary code. For example the if statement you use to check if the user specifically entered A4. That code is unnecessary because you will correctly calculate the value using the rest of your code. Unless this function is being called inside some tight inner loop, its speed isn't a concern, so that optimization is not very helpful.

Avoid Magic Numbers

Your code also has a bunch of magic numbers. These are hard-coded values in the code that have no indication what their purpose is. You should create named constants for these. You have one for 440Hz, but the values of the semitone offsets are a bit mysterious. In my code I put them in a table with a name so it's more obvious what they are. But in general, values other than 0 or 1 should have a name. I would also comment the relationship of 2^(numSemitones / 12) as it would be non-obvious to people who haven't studied the physics of Western music.

Nitpicks

One other thing I noticed is that in your code you didn't handle the cases of Fb, Cb, E#, or B#. These are valid notes that you'll want to handle. They don't come up often, and they are synonyms (at least in equal temperament) for E, B, F, and C, but they are legit, so I'd recommend handling them. (And if you want to get really technical, you could handle double sharps (Ax4, for example), and double flats (Bbb3, for example).)

\$\endgroup\$
  • 2
    \$\begingroup\$ A small pedantic point - C doesn't guarantee that 'B' - 'A' is 1, or that any of the other character arithmetic works as you want. In practice, the commonly used character codings such as ASCII and EBCDIC let you get away with this. Also, don't you have to divide by 12. rather than 12 (or change numOfSemitones back to a floating-point type), otherwise integer division will result in only two distinct values? I'm not sure how you have that working! \$\endgroup\$ – Toby Speight Aug 13 '18 at 7:50
  • \$\begingroup\$ @user1118321 The one thing I would change is to extract the frequency calculation and put that in a class 'Temperament'. Now you can have a visitor pattern for determining the frequency of a note, given the temperament. en.wikipedia.org/wiki/Musical_temperament. The reason I post this in a comment, rather than an answer is because I am a complete n00b in C. \$\endgroup\$ – dfhwze May 24 at 16:20
6
\$\begingroup\$

Bug: uninitialized variables

You didn't initialize sharp and flat to false, so they could randomly be true even when the note string doesn't contain any sharp or flat symbols. You should make a habit of initializing all your local variables to avoid these kinds of problems. Also, some compilers will warn you about uninitialized variables if you turn on full warnings, so you should look into how to do that for the compiler you are using.

\$\endgroup\$
4
\$\begingroup\$

The other two answers are good. I'll just throw in a few other ideas:

  1. It's a bit long, and your algorithm is fundamentally a few steps:

    • figure out semitone from A
    • calculate octave 4 frequency
    • adjust for active

    Therefore, your main function would be clearer if simply:

    int semitone     = calculateSemitone(note);
    double frequency = calculateA4freqency(semitone);
    double final     = adjustForOctave(frequency, note[3]);  
    return final
    

    You need just a couple of the 8 constants/temporary variables in your main function, so it's easier to follow.

  2. By separating out the flat/sharp variables, you've introduced two additional variables and if statements, which require 10 if statements to interpret. This adds to the 7 cases of the switch statement, for a rough complexity of 19 (2 + 10 + 7). If you don't have these variables, you can write a switch statement that handles the note itself: 'C', 'C#', 'Db', etc. This will be (I believe) 12 branches, therefore simpler. It will also be clearer from this how to make "table-driven" code per the other responses above.

  3. At first, I thought that if you instead base off of A0 (or C0), instead of A4, your octave calculation can be simpler-- instead of the if/elseif/else it can actually just be a single calculation. But upon further reflection, you can stick with your A4 if you trust the math to do the right thing... I believe you will find that octave = pow(2, (octave - 4)); final = round(frequency * octave); works correctly whether octave is <4, ==4 or >4. Try it!

This is a good first step, but it's always good to read back over and try to have each function do one thing, ideally with a clear input and output. Once you have smaller functions, it's easier to make them clean and readable.

\$\endgroup\$
3
\$\begingroup\$

Lots of good answers, just an additional note:

The string type from CS50 is considered bad practice and should never be used. This is because it expands to typedef char* string;. Hiding a pointer behind a typedef like this is bad practice. In addition, there is a type in C++ called string, so using that identifier turns the code mighty confusing.

For this reason alone, CS50 has earned a bad reputation among professional C programmers.

\$\endgroup\$
  • 1
    \$\begingroup\$ It's especially bad, as we'd prefer this function to accept const char* to promise we won't change the input string. \$\endgroup\$ – Toby Speight Aug 13 '18 at 20:57
3
\$\begingroup\$

Your calculation is much too complicated.

You start by inspecting note[1] without checking the length of the string, so your code is not robust. It would be better to inspect note[0] first.

The special case if(((key == 'A') && (octave == 4)) && ((!flat) && (!sharp))) should be eliminated. The general case calculation should work just fine. Once you eliminate the special case, key is used just once, so you can eliminate that variable in favour of note[0].

It doesn't make sense for numOfSemitones to be a float, and for octave to be a double. Those quantities should both be integral.

In music, it is acceptable to treat E♯ as an F, and C♭ as B. Therefore, you can generalize the handling of accidentals, so that # always raises the pitch by one semitone, and b always lowers the pitch by one semitone, regardless of the note letter.

The frequency = a4Frequency / pow(2, (numOfSemitones / 12)) calculation is written too many times. The special cases for A♯, B♭, and B could be handled using negative numbers for numOfSemitones, such that numOfSemitones consistently represents the number of semitones below A. (Actually, I would consider it more intuitive to count the number of semitones above A instead.)

You shouldn't need three cases to handle octave > 4, octave == 4, and octave < 4. (Again, take advantage of negative numebers!) Furthermore, if you roll the octave adjustment into numOfSemitones (counting 12 semitones per octave), then you wouldn't have to call pow() so many times.

Don't use final as a variable name. Choose something more meaningful, like freq.

Suggested solution

Here, I'm using a cleverer version of your octave = note[2] - '0' trick, but relative to '4' instead. As in your original code, though, that there is no validation of the octave number (particularly for octave 10 or above).

#include <math.h>

// Calculates frequency (in Hz) of a note
int frequency(string note)
{
    int semitones;      // Semitones relative to A4 (440 Hz)
    switch (note[0]) {  // First, assume octave 4
        case 'C': semitones = -9; break;
        case 'D': semitones = -7; break;
        case 'E': semitones = -5; break;
        case 'F': semitones = -4; break;
        case 'G': semitones = -2; break;
        case 'A': semitones =  0; break;
        case 'B': semitones = +2; break;
        default:  return 0; // Error!
    }
    switch (note[1]) {  // Adjust by 12 semitones per octave, and any accidental
        case '#': semitones += 12 * (note[2] - '4') + 1; break;
        case 'b': semitones += 12 * (note[2] - '4') - 1; break;
        default:  semitones += 12 * (note[1] - '4');
    }
    double freq = 440 * pow(2, semitones / 12.0);
    return (int)round(freq);
}
\$\endgroup\$
  • \$\begingroup\$ Hm. In your second switch, you assume the next char is a digit in the allowed range. You know what happens when you assume, right? Still much better. \$\endgroup\$ – Deduplicator Aug 13 '18 at 19:29
  • \$\begingroup\$ @Deduplicator It's just making the same assumption that was in the original code. \$\endgroup\$ – 200_success Aug 13 '18 at 19:30
  • \$\begingroup\$ Well, I went off the rest of your post, which among others stressed competent error-handling? \$\endgroup\$ – Deduplicator Aug 13 '18 at 19:32
  • \$\begingroup\$ @Deduplicator In my opinion, the important thing is not to crash. Validation is somewhat optional; I haven't gone out of my way to ensure that the octave number makes sense. \$\endgroup\$ – 200_success Aug 13 '18 at 19:37
  • \$\begingroup\$ You'll find that the character arithmetic works in EBCDIC for 'A'..'G' (the first discontinuity in letters is between 'I' and 'J'), and I'm not aware of any C targets with discontinuous 'A'..'G'. (It's good to point out the assumption though, and it definitely warrants a comment in the code. And - as I'm sure you know, but possibly not OP - C does guarantee that '0'..'9' are consecutive.) \$\endgroup\$ – Toby Speight Aug 13 '18 at 20:43
2
\$\begingroup\$

I believe if you want to write "cleaner looking" code you could implement a hashtable.

This answer goes over how to create a hashtable.

I think for your purposes it might be a little bit overkill, since your data set is limited. There's nothing wrong with having if-else inside your switch statements. One thing you could change is instead of adding

frequency = a4Frequency / pow(2, (numOfSemitones / 12));

in multiple places, having one line that handles the frequency calculation for all cases. You can accomplish this by checking the values of numOfSemitones and frequency against their initialized values after your code goes through the switch statement with

if (numOfSemitones != 0 && frequency != 0.0)  
/* Assuming we don't care if there is no frequency 
since we're calculating a present note */
{
    frequency = a4Frequency / pow(2, (numOfSemitones / 12));
}
else
{
    /* We did something wrong, throw an exception */ 
}

Since the A and B notes' frequencies are handled differently, I would recommend returning those via the switch statement, as you do with the base frequency of A4.

Additionally you could add more error checking with a default case, as well as checking the length of the note string and if the note[0] is a valid note letter.

\$\endgroup\$
  • \$\begingroup\$ How would you implement that line? I'm having trouble figuring out a way to initialise it so that it doesn't just return value before numOfSemitones is counted. Sound advice, thanks. \$\endgroup\$ – Karim Aug 11 '18 at 0:27
  • \$\begingroup\$ A hashtable for 7 possible letters would be overkill. Running all that code would almost certainly be slower than a switch. \$\endgroup\$ – 200_success Aug 13 '18 at 19:41
1
\$\begingroup\$
  1. The code is unnecessarily large. All you basically need to calculate pitch is: subtract 'A' from note, add octave multiplied by 12 (and check if note is more than 'B' so the octave should be one-less), change that number by one semitone if note is sharp or flat. Two additions, one branch, that's all.

  2. You have some strange logic with sharps and flats. Why do you ignore the second character in "Fb3"? Flats and sharps are merely modifications to pitch, they are not bound rigidly to black keys.

  3. The whole MIDI range consists of 128 notes and it greatly exceeds any live instrument range (and it is slightly less than what human's ear can perceive in principle); and note that it is even more than what you can encode using three-character strings, you only have 122 notes from Cb0 to B#9. You don't need to calculate pow each time, a small array of 122 carefully pre-calculated doubles would be much faster and more precise.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.