4
\$\begingroup\$

I am trying to understand good design patterns in Python and I cannot think of a way to break this huge function into smaller parts without making the code cluttered, overly complex or plain ugly.

I didn't want to clutter my question by posting the whole file. This function itself is already very large, but the class has only two methods: parse_midi() and generate_midi(file_name, file_length).

pitches, velocities, deltas, durations, and intervals are all MarkovChain objects. MarkovChain is a simple class with methods: add_event(event), generate_markov_dictionary(), and get_next_event(previous_event). MarkovChain.src_events is a list of events to generate the Markov chain from. It is a simple implementation of first order Markov chains.

def parse_midi(self):
    # on_notes dictionary holds note_on events until corresponding note_of event is encountered
    on_notes = {}
    time = 0
    previous_pitch = -1
    tempos = []
    delta = 0
    for message in self.track_in:
        time += message.time
        delta += message.time
        # There are also MetaMessages in a midi file, such as comments, track names, etc.
        # We just ignore them
        if isinstance(message, mido.Message) and message.type in ["note_on", "note_off"]:
            # some midi files use note_on events with 0 velocity instead of note_oof events
            # so we check if velocity > 0
            if message.velocity > 0 and message.type == "note_on":
                on_notes[message.note] = time
                self.pitches.add_event(message.note)
                self.velocities.add_event(message.velocity)
                self.deltas.add_event(delta)
                delta = 0
                if previous_pitch == -1:
                    self.intervals.add_event(0)
                else:
                    self.intervals.add_event(message.note - previous_pitch)
            else:
                # KeyError means note_off came without a prior associated note_on event!"
                # Just ignore them
                with ignored(KeyError):
                    self.durations.add_event(time - on_notes[message.note])
                    del on_notes[message.note]

            previous_pitch = message.note
        # Tempo might be many tempo changes in a midi file, so we store them all to later calculate an average tempo
        elif message.type == "set_tempo":
            tempos.append(message.tempo)
        elif message.type == "time_signature":
            self.time_signature = self.TimeSignature(message.numerator, message.denominator,
                                                     message.clocks_per_click, message.notated_32nd_notes_per_beat)
    # some tracks might be aempty in a midi file. For example they might contain comments as track name, and no note events
    if len(self.pitches.src_events) == 0:
        print("There are no note events in track {}!\n"
              "The file has {} tracks. Please try another one.".format(self.selected_track, self.num_tracks))
        exit(1)
    # a midi file might not contain tempo information at all. if it does, we calculate the average
    # else we just assign a default tempo of 120 bpm
    try:
        self.average_tempo = int(sum(tempos) / len(tempos))
    except ZeroDivisionError:
        self.average_tempo = mido.bpm2tempo(120)
\$\endgroup\$
  • \$\begingroup\$ We're all here to receive constructive criticism of our code. However, the current title of your question is very broad and doesn't really state the purpose of your code. We've found over time that questions with more descriptive titles get more attention from potential reviewers, so editing your question's title to describe what your code does will benefit both you and the people reviewing it. You may also want to read How to get the best value out of Code Review - Asking Questions. Thanks! \$\endgroup\$ – RubberDuck Mar 30 '15 at 11:37
1
\$\begingroup\$

I'm updating based on @veedrac's opinion and the doc.

You can split the method based on message.type. The logic behind message.type 'note_on' and 'note_off' is little unclear. So I handle the two in the same method __note_on_off.

To make the methods immune to inheritance override, I prepend double '_'.

Each method takes message object and a dict from parse_midias parameters.
__note_on_off is called only when message.type is either 'note_on' or 'note_off', so we can eliminate the check in if isinstance.

Because some of the splitted methods modify parse_midi's local objects those methods have to be defined inside parse_midi and make those names nonlocal or a dict from parse_midi has to be passed as argument to those methods. I pass a dict to maintain state.

def __note_on_off(self,message,d):
    d['time'] += message.time
    d['delta'] += message.time
    # There are also MetaMessages in a midi file, such as comments, track names, etc.
    # We just ignore them
    if isinstance(message, mido.Message):
        # some midi files use note_on events with 0 velocity instead of note_oof events
        # so we check if velocity > 0
        if message.velocity > 0 and message.type == "note_on":
            d['on_notes'][message.note] = d['time']
            self.pitches.add_event(message.note)
            self.velocities.add_event(message.velocity)
            self.deltas.add_event(d['delta'])
            d['delta'] = 0
            if d['previous_pitch'] == -1:
                self.intervals.add_event(0)
            else:
                self.intervals.add_event(message.note - d['previous_pitch'])
        else:
            # KeyError means note_off came without a prior associated note_on event!"
            # Just ignore them
            with ignored(KeyError):
                self.durations.add_event(d['time'] - d['on_notes'][message.note])
                del d['on_notes'][message.note]

        d['previous_pitch'] = message.note
    # Tempo might be many tempo changes in a midi file, so we store them all to later calculate an average tempo

def __time_sign(self,message,d):
        self.time_signature = self.TimeSignature(message.numerator, message.denominator, message.clocks_per_click, message.notated_32nd_notes_per_beat)

def __set_tempo(self,message,d):
    d['tempos'].append(message.tempo)

Then I build dispatch dict whose keys are message.type and values are the splitted methods.

I assume mido object is accessible to parse_midi and __note_on_off as I could not trace its root.

def parse_midi(self):
    ld = dict(on_notes = {},
              time = 0,
              previous_pitch = -1,
              delta = 0,
              tempos = []
             )

    dispatch = dict(
                    set_tempo=self.__set_tempo,
                    time_signature=self.__time_sign,
                    note_on=self.__note_on_off,
                    note_off=self.__note_on_off
                   )

    for message in self.track_in:
        try:
            dispatch[message.type](message,ld)
        except KeyError:
            print('Unkdown message type {0}'.format(message.type))
            sys.exit(1)                                                         

    # some tracks might be aempty in a midi file. For example they might contain comments as track name, and no note events
    if len(self.pitches.src_events) == 0:
        print("There are no note events in track {}!\n"
              "The file has {} tracks. Please try another one.".format(self.selected_track, self.num_tracks))
        exit(1)
    # a midi file might not contain tempo information at all. if it does, we calculate the average
    # else we just assign a default tempo of 120 bpm
    try:
        self.average_tempo = int(sum(ld['tempos']) / len(ld['tempos']))
    except ZeroDivisionError:
        self.average_tempo = mido.bpm2tempo(120)
\$\endgroup\$
  • \$\begingroup\$ aren't the values of on_notes = {} time = 0 previous_pitch = -1 delta = 0 going to be reset each time the for loop calls __note_on_offmethod? note_on with velocity=0 and a note_off event are both considered note_off events. And on_notes dictionary has to be persistent, because it keeps track of playing notes, whose note_off events may be several messages later, and during that time several notes may become on. If anything is not clear you can ask me other questions, or I can edit the original question to explain the situation further. \$\endgroup\$ – kureta Mar 30 '15 at 14:26
  • \$\begingroup\$ @kureta corrected to maintain state in __note_on_off. \$\endgroup\$ – Nizam Mohamed Mar 30 '15 at 16:17
  • \$\begingroup\$ thank you @nizam. I cannot upvote you since my rep is < 15 \$\endgroup\$ – kureta Mar 30 '15 at 16:27
  • \$\begingroup\$ I didn't test the code because you know I can't. Give us feedback if it's working. If works accept the answer. \$\endgroup\$ – Nizam Mohamed Mar 30 '15 at 16:56
  • \$\begingroup\$ Is there a reason you're using name mangling (__x) rather than just normal private methods (_x)? Also, I really don't like the idea of passing raw locals() to the function. There's no reason not to pass parameters properly and it's not even guaranteed to work. \$\endgroup\$ – Veedrac Mar 30 '15 at 19:18
2
\$\begingroup\$

Your notion of average tempo makes little sense. A track with four minutes of ♩=120 followed by a 12-second coda of ♩=60 would have a reported average tempo of 90. A more reasonable average would be to divide the total number of beats by the elapsed time.

$$\frac{120 \frac{\textrm{beats}}{\textrm{min}} × 4\ \textrm{min} + 60 \frac{\textrm{beats}}{\textrm{min}} × 0.2\ \textrm{min}} {4.2\ \textrm{min}} \approx 117 \frac{\textrm{beats}}{\textrm{min}} $$

\$\endgroup\$
  • \$\begingroup\$ Thanks, you are absolutely right. I already modify the tempo of resulting midi in a DAW afterwards, for now. But trying to find the reason of odd rhythms would have been a real headache after the application has become more functional. You saved me a lot of time there. In fact now I think that it would be a good idea to give weights to different tempos (tempi?) according to their duration and change the tempo of the resulting midi file probabilistically. Thanks again. \$\endgroup\$ – kureta Mar 30 '15 at 21:22

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.