New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Music submission #1

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@Jingxiang6
Copy link
Owner

Jingxiang6 commented Dec 30, 2018

No description provided.

Show resolved Hide resolved music/helpers.c Outdated
{
semitones = 0 + 12 * (note[2] - '0' - 4) - 1;
}
else if (note[1] != '#' && note[1] != 'b')

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Dec 30, 2018

Collaborator

There is a way to make these if-statements simpler. See if you can find it.

{
semitones = -2 + 12 * (note[1] - '0' - 4);
}
}

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Dec 30, 2018

Collaborator

The branches on each note repeat a bunch of common subexpressions. See if you can factor them out into variables.

else
{
return false;
}

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Dec 30, 2018

Collaborator

You can make this if-statement much simpler.

Jingxiang6 added some commits Jan 4, 2019

@@ -27,7 +27,7 @@ int frequency(string note)
{
semitones = 0 + 12 * (note[2] - '0' - 4) - 1;
}
else if (note[1] != '#' && note[1] != 'b')
else
{
semitones = 0 + 12 * (note[1] - '0' - 4);

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Jan 4, 2019

Collaborator

The indentation on this line and some other lines is off.

@@ -27,7 +27,7 @@ int frequency(string note)
{
semitones = 0 + 12 * (note[2] - '0' - 4) - 1;
}
else if (note[1] != '#' && note[1] != 'b')
else

This comment has been minimized.

@thejohnfreeman

thejohnfreeman Jan 4, 2019

Collaborator

What happens if note[1] == '#'?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment