Python script to test music sight reading

Question

I decided to write a program to test music theory, and, while it would've been much easier for me to make it elegant and perfect in Java, I thought I'd take the opportunity to get more familiar with Python.

Below is the full script. It simply prints a staff in treble of bass clef with a note somewhere and asks the user to identify it.

Things I'm particularly interested in getting feedback about:

1. The @staticmethod in the Staff class. I tried this both as an instance function (is it even appropriate to call it this in Python?) and as the static version below, but both seemed kind of clunky to me. It doesn't make much sense to me that I have to refer to the class's fields with the class name when I'm already in the class (e.g., Staff.lines instead of just lines, or self.lines if it's an instance function). It makes me think I'm doing something wrong.
2. I've heard multiple times that having constants isn't very Pythonic. Is there a better way for me to handle the TREBLE_CLEF, BASS_CLEF, TREBLE_CLEF_NOTES, and BASS_CLEF_NOTES fields?
3. I'm interested in a solution which allows the user to input a command like exit into the prompt in order to leave the program. I know I could've put this in the test_note function, but that would break the principle that each function should do only one thing and do it well (i.e., the test_note function test's the user's knowledge of the staff -- it shouldn't be responsible at all for controlling the application or its state). I couldn't think of an elegant way to do this and still preserve that principle, so I just bypassed the issue for now, haha.
4. Am I using classes appropriately? Any way I can use them better?

Of course, any and all feedback is welcome, especially including anything which breaks from conventional Python style. I'd like to be as Pythonic as possible.

---

import random
import time
class Clef:
 def __init__(self, name, notes):
 self.name = name
 self.notes = notes
 def random_note(self):
 return random.choice(list(self.notes.keys()))
 def get_note(self, index):
 return self.notes[index]
class Staff:
 lines = 9
 line_length = 9
 line_text = '-' * line_length
 space_text = ' ' * line_length
 note_text = 'O'
 @staticmethod
 def display(note_index):
 for i in range(Staff.lines):
 add_note = i == note_index
 print_line = Staff.line_text if i % 2 == 0 else Staff.space_text
 if i == note_index:
 print_line = print_line[:4] + Staff.note_text + print_line[5:]
 print(print_line)
TREBLE_CLEF_NOTES = { 0 : 'F',
 1 : 'E',
 2 : 'D',
 3 : 'C',
 4 : 'B',
 5 : 'A',
 6 : 'G',
 7 : 'F',
 8 : 'E' }
BASS_CLEF_NOTES = { 0 : 'A',
 1 : 'G',
 2 : 'F',
 3 : 'E',
 4 : 'D',
 5 : 'C',
 6 : 'B',
 7 : 'A',
 8 : 'G' }
TREBLE_CLEF = Clef('Treble', TREBLE_CLEF_NOTES)
BASS_CLEF = Clef('Bass', BASS_CLEF_NOTES)
def test_note(clef, note_index):
 print(clef.name)
 Staff.display(note_index)
 answer = clef.get_note(note_index)
 user_choice = input('Enter the note: ').strip().upper()
 if user_choice == answer:
 print('Correct!')
 else:
 print('Wrong. This is ' + answer)
def main():
 print('To exit, type CTRL + C')
 while True:
 print('\n')
 clef = random.choice([TREBLE_CLEF, BASS_CLEF])
 test_note(clef, clef.random_note())
 time.sleep(2)
if __name__ == '__main__':
 main()

Answer 1

To address your numbered questions:

1. A class with one static method has no reason to be a class at all. You could write a function instead. If you need multiple functions to share some data, the normal approach is to use instance methods and instance attributes of a class. Only when you need multiple instances of a class to share some data, use class attributes.

2. There is a naming convention for constants in Python and it is exactly what you use: all caps with underscores between words. I don't there would be a convention for something that is strongly discouraged.

   When I see constants in a module, I understand that if I want to change them I have two options: either edit them to change them for good, or look up their use in the code and implement another approach. In both cases the constant is a good starting point.

3. each function should do only one thing and do it well (i.e., the test_note function tests the user's knowledge of the staff -- it shouldn't be responsible at all for controlling the application or its state)

   I don't think testing user's knowledge is only one thing from the programmer's perspective. The test_note function presents the problem to the user, reads input, decides if the answer is correct and outputs the result. Perhaps you could create a class to handle some of that:

   class NoteTest(object):
    def __init__(self):
    self.clef = random.choice([TREBLE_CLEF, BASS_CLEF]) 
    self.note = self.clef.random_note()
    def print_test(self):
    print(self.clef.name)
    Staff.display(self.note)
    def print_result(self, user_choice):
    answer = self.clef.get_note(self.note) 
    if user_choice == answer:
    print('Correct!')
    else:
    print('Wrong. This is ' + answer) 
   def main():
    while True:
    note_test = NoteTest()
    print('\n')
    note_test.print_test()
    user_choice = input('Enter the note or exit to exit: ').strip().upper()
    if user_choice == 'EXIT':
    return
    note_test.print_result(user_choice)
    time.sleep(2)
   

4. See the answers to 1 and 3.

---

To replace the Staff class I propose this function. Note the use of str.center instead of the less convenient print_line[:4] + Staff.note_text + print_line[5:]

def print_staff(note_index, 
 lines=9, 
 line_length=9, 
 line_text='-',
 space_text=' ',
 note_text='O'):
 for i in range(lines):
 fill_char = (line_text, space_text)[i % 2]
 symbol = note_text if i == note_index else ''
 print(symbol.center(line_length, fill_char))

• asteri: Thanks for your comments! Re: A class with one static method has no reason to be a class at all, the reason I thought a class made sense is because it encapsulated all of the fields/settings for the Staff object (lines, text, etc.). Originally, I had them all as constants, but it seemed dirtier. Do you disagree?
• Janne Karila: @JeffGohlke Added a few words about constants, and a concrete replacement for Staff

Answer 2

Warning for other reviewers : input and input are different depending on the version of Python you are using. This is about Python 3.

---

Instead of defining the notes of a clef as a dictionnary mapping consecutive integers from 0 to n to names, you could use a list.

You'll have the much more natural :

TREBLE_CLEF_NOTES = [ 'F', 'E', 'D', 'C', 'B', 'A', 'G', 'F', 'E' ]
BASS_CLEF_NOTES = [ 'A', 'G', 'F', 'E', 'D', 'C', 'B', 'A', 'G' ]

and you just need to change :

def random_note(self):
 return random.choice(range(len(self.notes)))

---

add_note = i == note_index does not seem useful : get rid of it.

---

The way you are playing with string slicing to display a note or a line doesn't need to be that complicated. Also, defining values as class members was a nice touch but the fact that you hardcode indices in the logic with the string slicing kind of removes the all point. As for me, I'll keep things simple and not so well organised for the time being as these values are unlikely to be useful for anything else than displaying.

Here's my suggestion :

def display(note_index):
 for i in range(9):
 line_element = '-' if i%2 else ' '
 print(line_element * 4 + ('O' if i == note_index else line_element) + line_element*5)

---

I have to go, I'll try to add more details later. In the meantime, if you are interested in Python and music theory, you might be interested by this github project.

• asteri: Ah, yeah, the add_note variable was supposed to be removed. Whoops. And as for the string slicing, you're right, that should be more dynamic based on the line_length variable. Whoops again! Haha. Thanks for your comments.
• asteri: I think changing the dictionaries to lists would require a lot more changes to the code than just the random thing, though.