pset3 Music design and connection to Week 2 lecture?

Question

I have completed all of the work for pset3 Music and it works fine. There are no error messages, I can synthesize the songs, and it passes all the Check50 tests. However I can't shake this nagging feeling that my code is not designed well, especially for the frequency function. There must be a more efficient way to write this code than what I have done.

I am also having a very tough time connecting the solutions for this problem to the topics discussed in the Week 2 lecture. They seem to be disconnected. Are there any ways to use the sorting concepts or recursion that David discussed in this problem?

Here is my code for the helpers functions:

// Helper functions for music

#include <cs50.h>
#include <ctype.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "helpers.h"

// Converts a fraction formatted as X/Y to eighths
int duration(string fraction)
{
    int result;
    int n = fraction[0] - '0';
    int d = fraction[2] - '0';

    result = 8 * n / d;

    return result;

}

// Calculates frequency (in Hz) of a note
int frequency(string note)
{
    //set variables
    float hertz = 440;
    char key[2];
    int accidental = 0;
    char octave[2];
    int o = 0;
    int adjust = 0;

    //save note letter as key
    key[0] = toupper(note[0]);
    key[1] = '\0';

    //save accidentals and octaves
    if (note[1] == '#')
    {
        accidental = 1;
        octave[0] = note[2];
    }
    else if (note[1] == 'b')
    {
        accidental = -1;
        octave[0] = note[2];
    }
    else
    {
        octave[0] = note[1];
    }

    octave[1] = '\0';

    //adjust for octave
    o = atoi(octave) - 4;
    hertz = hertz * pow(2, o);


    //adust for accidental
    hertz = hertz * pow(2, (accidental / 12.0));


    //value for keys
    if (key[0] == 'B')
    {
        adjust = 2;
    }

    if (key[0] == 'C')
    {
        adjust = -9;
    }

    if (key[0] == 'D')
    {
        adjust = -7;
    }

    if (key[0] == 'E')
    {
        adjust = -5;
    }

    if (key[0] == 'F')
    {
        adjust = -4;
    }

    if (key[0] == 'G')
    {
        adjust = -2;
    }

    //adjust for keys
    hertz = hertz * pow(2, (adjust / 12.0));

    //return
    return round(hertz);

}

// Determines whether a string represents a rest
bool is_rest(string s)
{
    if (strcmp(s, "") == 0)
    {
        return true;
    }
    else
    {
        return false;
    }
}

Answer 1

If you allow me, I have a suggestion or two:

• Keys, Octaves and Accidentals can be char rather than an array of char since they are always one character only. So instead of representing each of them as an array of char where the second element is a \0 just char octave = note[strlen(note) - 1] will always give you the last character of the note, which is always the octave.

• Same with key... char key = note[0] will always be the key of any note.

• Instead of multiple if statements, a switch could be cleaner and easier to read:

switch(key) {
    case(key == 'B'):
        adjust = 2;
        break;

    case(key == 'C'):
        adjust = -9;
        break;

    .......    

    default:
        eprintf("Invalid key? Found: %s", key);
}