2

I want to manage music notes in a C# program, and I wrote a couple classes for that. However, I think I'm having trouble respecting the DRY principle. Either this or I'm overthinking things. Sorry if the question has already been treated somewhere else, I couldn't think of more explicit keywords and yet found nothing about my problem.
I couldn't find a simpler example which would fit the explanation of my problem, and I didn't know what to include and what to leave out of my question, so I wrote a quick version and a detailed version of the question.

Background

Let's say there exist seven labels to describe all musical notes (letters from A to G). Of course, there are more than seven notes to any composition, so only seven labels weren't enough. So we started putting them on shelves (octaves). A note could share the same label as another, but could be on a different shelf (a higher one or a lower one). This is why we find C notes that are higher than other C notes.
Because of all of this, labeling notes is non-trivial in computer representation: there are two separate informations (label and shelf), labels wrap around when a note shifts from a shelf to another, and that has to be managed.

I have the following class which encapsulates both informations about label and octave:

public class OctaveNote
{
    // Note is an enum storing my seven labels.
    private Note n;
    private byte o;

    public OctaveNote(Note note, byte octave)
    {
        n = note;
        o = octave;
    }
}

I also have these two member functions which are used to pitch a note up or down:

public OctaveNote PitchUp(byte tones)
{
    byte octaveShift = (byte)(tones / 7);
    byte newOct = (byte)(o + octaveShift);

    char noteShift = (char)(tones % 7);
    Note newNote = (Note)((((int)(n + noteShift) % 7) + 7) % 7);
    if (newNote < n)
    {
        newOct++;
    }

    return new OctaveNote(newNote, newOct);
}

public OctaveNote PitchDown(byte tones)
{
    byte octaveShift = (byte)(tones / 7);
    byte newOct = (byte)(o - octaveShift);

    byte noteShift = (byte)(tones % 7);
    Note newNote = (Note)((((int)(n - noteShift) % 7) + 7) % 7);
    if (newNote > n)
    {
        newOct--;
    }

    return new OctaveNote(newNote, newOct);
}

The problem

The code is a bit complicated so let's say the logic isn't relevant for now. The real problem is here: the two methods are identical, except for three statements:

n + noteShift    vs.    n - noteShift
newNote < n      vs.    newNote > n
newOct++         vs.    newOct--

I would like to merge the two methods to avoid code duplication. tones would not be a byte but a char, and could take negative values to express pitching downwards, like so:

n.Pitch(3)   => n.PitchUp(3)
n.Pitch(-4)  => n.PitchDown(4)

The naive answer would be to throw in a few if statements here and there to test whether tones is negative, and perform operations accordingly, but isn't there a more elegant way? That is, is it possible to write a single fragment code that behaves in a different way when tones < 0?


The number 7 is being used a lot because (in this case) an octave is comprised of 7 tones. It is used to find the octave shift (integer division) and the note shift (mod).

The only tricky part is the assignment of newNote, because n - noteShift can be negative and the mod may produce an invalid value to cast back into my enum, so I worked around it by adding 7 before mod-ing again.

For non-musicians out there:

  • Notes are represented by letters from A to G and wrap around on both ends: A B C D E F G A B C...
  • Thus, there can be different notes with the same label, for example a C note higher than another C note.
  • In order to distinguish these, we have octaves. For example, C5 is one octave above C4.
  • Octaves shift upon reaching the C note. This is (partly) because the most well-known scale starts on C, and this is why my enum starts on C as well. The following sequence is notes in their natural order: ...G3 A3 B3 C4 D4 E4 F4 G4 A4 B4 C5 D5...
  • 2
    How do you represent the note C#? (Pun intended). You should try to reduce your question a lot, focus only on the PitchUp and PitchDown methods. In the end your question might be a better fit on StackOverflow, if so it will be moved. It seems you are doing something very simple in a extremely complicated way. – Bent Dec 27 '17 at 18:30
  • Looks like you are hard coding 7 notes per octave. There are 7 white key notes in an octave, but there are 12 total notes in an octave. I think modeling only the white keys will become problematic and not have much usefulness. Seems likely this all being done to handle transposing, and doing that without the entire set of 12 notes per octave is going to cause the transposed melody to shift in modality. For all practical purposes, will not result in a usable transposition. – Thomas Carlisle Dec 27 '17 at 19:03
  • @Bent In the full version of my enum, C# is labeled CS, as in "C Sharp" ;) And yes, you're right, that question is too long. I'll try to trim it as much as I can, but I tried to prevent such incovenience by writing a quick version and a detailed version. Anyway, I've always had trouble not getting lost into detail when explaining something :( I'll work on it! @ThomasCarlisle Yeah, I voluntarily omitted black keys for the sake of explaining that whole stuff to non-musician people without confusing them more than needed. Don't worry, I do have the full version, with the 5 sharp notes :) – qreon Dec 27 '17 at 19:13
  • 1
    The opinions of Martin Fowler and Kent Back are that code should read familiar to the domain expert. In OP's case, OP is the domain expert (in musical theory), so as long as the code runs correctly, OP should choose whatever is most familiar. Of course OP should ask other domain experts (in musical theory, and preferably also in musical software development) to review the code and get recommendations. An objection to the code that comes from a pure software programmer (without backgrounds in musical theory) should be relegated to a lower priority. – rwong Dec 27 '17 at 19:55
  • @ThomasCarlisle: And 12 notes is still not enough, because C Sharp and d flat are two different notes that sound the same (or almost the same; there are keyboards that will actually play them differently). – gnasher729 Dec 29 '17 at 11:14
5

What you are describing sounds like you want something similar to the relationship between a DateTime and a TimeSpan. Perhaps you could describe it as a Note and a NoteShift. I think that calling it a Note instead of an OctaveNote is much clearer when you consider that C7 is a different note than C4 (C in the 7th octave vs C in the 4th).

Further learning from the relationship between a DateTime and a TimeSpan, you could reduce the internal complexity of the math by storing the note as an integer. Whether that integer represents a frequency or a position on the scale is up to you. The concept of how to represent the Note is done in the formatting functions.

Technically speaking there are 12 notes in the modern scale with A4 being 440Hz. Each octave represents a doubling of the frequency so that A5 is 880Hz and A3 is 220Hz. Those are representative details you can incorporate as a method to get the frequency from the note.

Let's say you have to represent 8 octaves (88 key keyboard starts at A0 and ends at C8). You could represent all of that as a number between between 0 and 107. This makes the math much simpler.

public class Note
{
    public byte MaxOctave = 8;
    public enum Name { C, Csharp, D, Dsharp, E, F, Fsharp, G, Gsharp, A, Asharp, B };
    private final byte note;

    public Note(Name name, byte octave)
    {
        note = ((octave + 1) * ((byte) name)) - 1;
    }

    private Note(byte newNote)
    {
        if (newNote < 0 || newNote > ((byte)Enum.GetNames(typeof(Name)).Length * MaxOctave))
        {
            throw new ArgumentOutOfRangeException(nameof(newNote));
        }

        note = newNote;
    }

    public Note ChangePitch(NoteShift shift)
    {
        return new Note(note + shift.relativeNote);
    }

    public override string ToString()
    {
        var notesPerOctave = (byte)Enum.GetNames(typeof(Name)).Length;
        var nameVal = note % notesPerOctave;
        var octave = note / notesPerOctave;

        var name = (Name)Enum.GetValue(typeof(Name), nameVal);
        return $"{name}{octave}";
    }
}

Your NoteShift would just have a positive byte or a negative byte for the amount to shift. You have the math complication in the ToString() call, but typically that doesn't get called all the time.

public class NoteShift
{
    final byte relativeNote; // package access

    public NoteShift(byte shift)
    {
        relativeNote = shift;
    }

    public static NoteShift AddNotes(byte notes)
    {
        return new NoteShift(notes);
    }

    public static NoteShift AddOctaves(byte octaves)
    {
        return new NoteShift(octaves * (byte)Enum.GetNames(typeof(Note.Name)).Length);
    }
}

A further benefit of this arrangement is that you can employ some intelligent operator overloading for addition, subtraction, greater than, less than, and equals. As well as implementing IComparable<Note> and IEquatable<Note>. That can make your library a lot more expressive. Just make sure the type of operators you overload make sense. There is no real reason to multiply or divide notes for example.

  • I don't know exactly what I was hoping for when I clicked the "Ask question" button, but your solution fits my need so well that I can't see how I didn't think of it before. Thanks a lot! :) – qreon Dec 27 '17 at 20:36
  • You're quite welcome. – Berin Loritsch Dec 27 '17 at 20:44
  • Curious why the downvote? Obviously the OP said this was exactly what they needed so I did answer the right question. I'm also pretty well versed in music theory. – Berin Loritsch Dec 27 '17 at 21:28
  • I've been downvoted as well haha. Probably shouldn't pay attention. – qreon Dec 27 '17 at 23:16
  • Everyone was, I always want to be open to constructive criticism, it's how we grow. – Berin Loritsch Dec 27 '17 at 23:55
1

The most elegant way to do this is to represent an OctiveNote as an integer. This is in swift but it should be easy to translate:

enum Note: Int {
    case c, d, e, f, g, a, b
}

struct OctiveNote {
    let rawValue: Int

    init?(rawValue: Int) {
        precondition(rawValue >= 0)
        self.rawValue = rawValue
    }

    init(n: Note, o: Int) {
        precondition(o >= 0)
        rawValue = o * 7 + n.rawValue
    }

    var note: Note {
        return Note(rawValue: rawValue % 7)!
    }

    var octive: Int {
        return rawValue / 7
    }
}

With the above, your pitch functions are merely addition and subtraction.

extension OctiveNote {
    func pitched(byTones tones: Int) -> OctiveNote {
        precondition(rawValue + tones >= 0)
        return OctiveNote(rawValue: rawValue + tones)!
    }
}

Now you can:

var n = OctiveNote(n: .a, o: 5) // n = A5
n = n.pitched(byTones: 1)       // n = B5
n = n.pitched(byTones: 1)       // n = C6
n = n.pitched(byTones: 1)       // n = D6
n = n.pitched(byTones: -3)      // n = A5
  • Quite similar to the accepted answer, except it's Swift :( – qreon Dec 28 '17 at 0:06
  • 1
    Yea, fortunately this is SE not SO. The important thing is the algorithm, not the language it's programmed in. The accepted answer has a better explanation too, which is important. :-) – Daniel T. Dec 28 '17 at 0:10
0

You can replace PitchUp and PitchDown with a single function. Add a second parameter that tells which direction to change the pitch. For example:

public enum PitchDirection
{
    Up,
    Down
}

public OctaveNote ChangePitch(byte tones, PitchDirection direction)
{
    byte octaveShift = (byte)(tones / 7);
    byte newOct = direction == PitchDirection.Up
                    ? (byte)(o + octaveShift)
                    : (byte)(o - octaveShift);

    char noteShift = (char)(tones % 7);
    char temp = direction == PitchDirection.Up
                    ? n + noteShift
                    : n - noteShift;
    Note newNote = (Note)((((int)(temp) % 7) + 7) % 7);
    if (direction == PitchDirection.Up && newNote < n)
    {
        newOct++;
    }
    if (direction == PitchDirection.Down && newNote > n)
    {
        newOct--;
    }

    return new OctaveNote(newNote, newOct);
}

I haven't tested this (just whipped it up off the top of my head), but it should point you in the right direction.

  • or do it with one parameter with a positive shift value meaning to shift up, and negative means to alter pitch down. – Thomas Carlisle Dec 27 '17 at 18:51
  • 1
    As per @ThomasCarlisle flags are bad. See fro example martinfowler.com/bliki/FlagArgument.html – Peter M Dec 27 '17 at 19:00
  • @MikeHofer if that's the only solution then I'll go with it, but I was looking for something a lot more abstract than throwing in if statements and ternaries to branch the correct execution path depending on a parameter's value (be it as a flag, as you suggested, or implied within a parameter's range, as suggested by @ThomasCarlisle) @PeterM thanks for the link, that shines some light on my questioning :) – qreon Dec 27 '17 at 19:19
  • @PeterM He argues flags are bad because they obscure the meaning of the call. The use of an enum makes it quite clear in this case - you'll be using "Up" or "Down" rather than "true" or "false" – mmathis Dec 27 '17 at 19:36
  • @qreon elegant is all fine and good, but you really want maintainable and readable before elegant. Doing this all in one line is no use when a bug is discovered and none of the developers at the company since you left understand what the method is doing. – mmathis Dec 27 '17 at 19:45

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.