Music player written in Python using PyQt

Some days ago, in the middle of a flight, in one hour or so, I've written this basic music player using Python3 and PyQt4.

I needed some easy to use music player, so I decided to write this piece of code that just works.

This little guy is called Frederic. It uses, besides PyQt for the GUI, a library called mutagen to work with tags in the music files.

This is my first time with Code Review, so I decided to publish this little piece of code to try it.

Any criticism, suggestion and opinion is welcome. Thanks!

frederic.py:

```
import sys
from PyQt4 import QtCore, QtGui
from PyQt4.phonon import Phonon
from mutagen.id3 import ID3

class Frederic(QtGui.QDialog):

def __init__(self):
QtGui.QWidget.__init__(self)

vbox = QtGui.QVBoxLayout(self)

self.setWindowTitle("Frederic Music Player")
self.move(400, 200)

self.label = QtGui.QLabel(self)
self.status = QtGui.QLabel(self)
self.play = QtGui.QPushButton("Play")
self.pause = QtGui.QPushButton("Pause")
self.stop = QtGui.QPushButton("Stop")
self.siguiente = QtGui.QPushButton("Siguiente")
self.anterior = QtGui.QPushButton("Anterior")
self.listWidget = QtGui.QListWidget(self)

self.agregar = QtGui.QPushButton("Agregar")
self.eliminar = QtGui.QPushButton("Eliminar")
self.artist = None
self.song = None

# Phonon settings

self.player = Phonon.createPlayer(Phonon.MusicCategory)
self.player.setTickInterval(100)
self.player.tick.connect(self.ticking)
self.spin = Phonon.SeekSlider(self.player, self)

# Implementing the layout

vbox.addWidget(self.label)
vbox.addWidget(self.status)

hbox0 = QtGui.QHBoxLayout(self)

hbox0.addWidget(self.play)
hbox0.addWidget(self.pause)
hbox0.addWidget(self.stop)
hbox0.addWidget(self.anterior)
hbox0.addWidget(self.siguiente)

vbox.addLayout(hbox0)

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. Perhaps something like this:

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.

StackExchange Code Review Q#152528, answer score: 4