0

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, I mean 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)
kureta
  • 478
  • 4
  • 15
  • 3
    Have you thought about posting this on codereview.stackexchange.com? – Alexander Craggs Mar 30 '15 at 11:03
  • I had no idea that it existed. Should I post the whole script or just this function? Thanks very much for the tip. – kureta Mar 30 '15 at 11:05
  • I would recommend just posting this section. If people who answer it need the whole code, they'll ask. – Alexander Craggs Mar 30 '15 at 11:07
  • 1
    codereview.stackexchange.com is indeed a better place for this kind of questions - there are many valid ways to refactor your function and it's mostly a matter of opinion. As far as I'm concerned (and not having worked on your project so I don't have enough background / hint), I'm not sure this function actually _needs_ any refactoring , except for the handling of empty files which should raise a (custom) exception so the calling code can decide how to handle the case. Else, the function is indeed a bit long but not over the top and seems coherent (doesn't do 1000 different things AFAICT). – bruno desthuilliers Mar 30 '15 at 11:31
  • Thanks for your response. So it seems that the _uglyness_ is inherent in what I'm trying to do. And you are right, custom exception is the way to go. thanks again. – kureta Mar 30 '15 at 12:30

1 Answers1

0

It turns out there is not much to refactor in this method, however, the best attempt to answer this question can be found here

Community
  • 1
  • 1
kureta
  • 478
  • 4
  • 15