6
\$\begingroup\$

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)
\$\endgroup\$
  • \$\begingroup\$ What about sharps/flats and different octaves? \$\endgroup\$ – el.pescado Dec 7 '14 at 11:50
6
\$\begingroup\$
  • __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)
\$\endgroup\$
5
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ Python 3 doesn't need object; all classes are new-style by default \$\endgroup\$ – jonrsharpe Dec 6 '14 at 15:10
  • \$\begingroup\$ Wow, thanks @jonrsharpe for that, good to know \$\endgroup\$ – janos Dec 6 '14 at 15:12
3
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Named constants are certainly appropriate for things which could be changed without rendering a program meaningless. To my eye, however, they are not always appropriate in cases where they aren't just used as numbers but also form part of the structure of a program or its algorithms. Within the context of musical note processing, it would take less thinking for me to recognize the numerical literals 7 and 12 as being more recognizable the number of letter names per octave and half-steps per octave, respectively, than for me to recognize LetterNamesPerOctave and HalfStepsPerOctave. \$\endgroup\$ – supercat Dec 6 '14 at 22:11
  • \$\begingroup\$ A concern I would have using a named constant for LetterNamesPerOctave is that such usage might imply that code could be easily adapted to something like German note names (German uses letters A-H, but doesn't use eight note letters per octave; a C major scale in German would be C-D-E-F-G-A-H-C, while F major would be F-G-A-B-C-D-E-F). If the relationship between letter names and chromatic note numbers is baked into the program structure, numeric literals would simply be a part of that. \$\endgroup\$ – supercat Dec 6 '14 at 22:17

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.