Music note class

Question

A class for notes calculation.

#!/usr/bin/env python3

from string import ascii_uppercase


class Notes:
    notes = ascii_uppercase[:7]
    notes_number = range(7)
    notes_dict = dict(zip(notes, notes_number))
    numbers_dict = dict(zip(notes_number, notes))

    def __init__(self, _note):
        if not _note in Notes.notes:
            raise Exception("Not a valid note")
        self.note = _note
        self.note_number = Notes.notes_dict[self.note]

    def add(self, n):
        if not n in range(8):
            raise Exception("Not a valid number")

        old_number = Notes.notes_dict[self.note]
        new_number = (old_number + n) % 7
        return Notes.numbers_dict[new_number]

    def minus(self, n):
        if not n in range(8):
            raise Exception("Not a valid number")

        old_number = Notes.notes_dict[self.note]
        new_number = (old_number - n) % 7
        return Notes.numbers_dict[new_number]

Test:

x = Notes("G")
x.add(7)

Answer 1

• __init__ assigns self.note_number, but the other functions don't use it, instead they look up the number from the dict. Choose one approach and eliminate the other.
• Checking the range of n and raising the exception in add and minus is not necessary, because the computation has a valid result for all numbers.
• add and minus are almost the same. One function that allows a negative argument would suffice. If you want to keep minus implement it simply as return self.add(-n)

Answer 2

I recommend to rename the class from Notes to Note, as plural suggests a collection of notes, when in fact your implementation is for individual notes.

---

PEP8 suggests to use x not in items instead of not x in items, so instead of this:

if not n in range(8):

This is the recommended way:

if n not in range(8):

---

notes and notes_number are not great names, and the second could be derived from the first, like this:

class Note:
    valid_note_letters = ascii_uppercase[:7]
    valid_note_numbers = range(len(valid_note_letters))

---

This is not efficient:

    if not n in range(8):

In the worst case, it will iterate over all 8 elements of the range. It would be better this way:

if not 0 <= n < 8:

This is a simple range check with precisely 2 comparisons.

---

A custom __str__ implementation would be user-friendly:

def __str__(self):
    return self.note

Answer 3

Your code is full of 7 and 8, numbers like that are called magic numbers and should be avoided. Instead use a constant.