Return to Revisions

It can be a pleasure to whip something up in an hour on the plane. This code could be improved by using pylint.

The most obvious improvements appear to be

• minimize and structure the instance variables
• refactor code to remove repetition
• corner case testing
• make styling standard and consistent

There are so many unstructured instant variables that it obscures the structure, some errors and the fact that many are not needed.

As a small example, of the following variables:

self.file_name = None
self.data = None
self.songs = {}
self.SongPlaying = None
self.SongQueued = None

only self.songs is actually used as anything but a local variable. Similarly, none of the buttons need to be instance variables. In the UI only three widgets are actually referenced outside of setup.

Where possible prefer self-documenting code. The songs variable might better be called filename_by_track_label.

There is a good amount of code repetition. For example, funcPlay, nextSong and previousSong all contain duplicated code. It could be factored out into something such as

def play_song(self, song_index):
    """Play song at :song_index: in UI list"""
    next_track_label = self.ui.update_label_by_index(song_index)
    next_filename = self.filenames_by_track_label[next_track_label]
    self.player.setCurrentSource(Phonon.MediaSource(next_filename))
    self.player.play()
    self.ui.songs_list_widget.setCurrentRow(song_index)

You could further remove code duplication in the UI construction by use of a helper, for example

def help_add(container, widget_list):
    for widget in widget_list:
        container.addWidget(widget)

hbox0 = QtGui.QHBoxLayout(self)
help_add(hbox0, (play_btn, pause_btn, stop_btn, anterior_btn, siguiente_btn))

hbox = QtGui.QHBoxLayout(self)
help_add(hbox, (agregar_btn, eliminar_btn))

vbox = QtGui.QVBoxLayout(self)
help_add(vbox, (self.label, self.status, hbox0, self.songs_list_widget, hbox, spin))

There are a number of bugs lingering in the code

• all next and previous track computations will overflow. Computation should be modulus number of tracks

    song_index = (self.ui.current_song_number() + 1) % self.ui.song_count()
  

• track label formation catches exceptions but (a) uses a bare except, (b) largely repeats the try block in the exception handler, (c) inserts the malformed label and (d) includes a dangling variable name

    try:
        id3 = ID3(file_name)
        data = id3['TPE1'].text[0] + " - " + id3["TIT2"].text[0]
        self.filenames_by_track_label[data] = file_name
        self.ui.listWidget.addItem(data)
    except (IOError, LookupError):
        self.ui.show_error("El archivo que ha elegido no funciona. Seleccione otro archivo por favor.")
  

You might consider separating the UI from the controller to provide some structure. I have shown some elements of that in my examples with self.ui

Of course in an hour you might not apply completely consistent styling but pylint can help you be disciplined. Applying its recommendations would further improve the code.