3
\$\begingroup\$

Background Info

I am an intermediate level Python programmer. This is my implementation of a MP3 Playlist class challenge that was hosted on 101 Computing.Net. The goal of the challenge was to implement a Class in Python to maintain an MP3 Playlist. The key features of the class are the ability to:

  • Enqueue an MP3 to the playlist
  • Remove an MP3 from the playlist
  • Load a playlist from a text file
  • Save a playlist on a text file
  • Shuffle all the songs in the playlist
  • Display all the tracks from the playlist
  • Count the number of tracks in the playlist
  • Calculate the total duration of the playlist
  • Empty/Reset the playlist
  • Check if the playlist is empty

MP3PLaylist Class

This is the mp3playlist.py file which contains the MP3Playlist class.

import os
import sys
import datetime
from random import shuffle
from track import Track


class MP3PlayList:
    """A MP3Playlist class."""

    def __init__(self, title: str, filename: str) -> None:
        """Initialize attributes."""
        self.title = title
        self.filename = filename
        self.tracks = []

    def __repr__(self) -> str:
        """String representation the class."""
        return f'{self.__class__.__name__}(title={self.title}, filename={self.filename}, tracks={self.tracks})'

    def load_playlist(self):
        """Loads a saved playlist, and displays its contents"""
        if not os.path.isfile(os.path.join('music_files', self.filename)):
            print(f'{self.title} could not be loaded.\n{self.filename} does not exists!', file=sys.stderr)
        else:
            with open(os.path.join('music_files', self.filename)) as file_obj:
                song_list = [i.strip().split(',') for i in file_obj]
            for song in song_list:
                self.add_track(Track(song[0], song[1], song[2]))
            self.show_playlist()

    def save_playlist(self, filename=None):
        """Saves the current playlist in a textfile"""
        self.filename = self.filename if filename is None else filename

        with open(os.path.join('music_files', self.filename), 'w+') as file_object:
            for track in self.tracks:
                song, artist, duration = track.title, track.artist, track.duration
                file_object.write(f'{song},{artist},{duration}' + '\n')
        print(f'{self.title} has been successfully saved.')

    def add_track(self, track):
        """Add a single track to the playlist."""
        self.tracks.insert(0, track)
        print(f'"{track.title}" has been added successfully!')

    def remove_track(self, track_name):
        """Removes a track from the playlist"""
        is_located = False
        for i, track in enumerate(self.tracks):
            if track_name == track.title:
                self.tracks.pop(i)
                print(f'"{track_name}" has been successfully removed.')
                is_located = True
        if not is_located:
            print(f'"{track_name}" could not be located. Please try again.')

    def show_playlist(self):
        """Displays entire playlist, including title, artist, and duration."""
        if self.tracks:
            print('Track list:')
            for track in self.tracks:
                song, artist, duration = track.title, track.artist, track.duration
                print(f'\t• {song} - {artist} ({duration})')
        else:
            print('Track list is empty!')

    def shuffle_playlist(self, n):
        """Shuffles playlist n x"""
        if self.tracks:
            for i in range(n):
                shuffle(self.tracks)
            print(f'Playlist was shuffled {n}xs!')
        else:
            print('Playlist could not be found.')

    def get_number_of_tracks(self):
        """Prints the total number of tracks stored in the playlist."""
        print(f'Total tracks: {len(self.tracks)}')

    def get_total_duration(self):
        """Prints the total duration time of all tracks."""
        current_year = datetime.datetime.now().year
        datetime_original = datetime.datetime(year=current_year, month=1, day=1)
        for track in self.tracks:
            minutes, seconds = [int(i) for i in track.duration.split(':')]
            datetime_original += datetime.timedelta(minutes=minutes, seconds=seconds)
        print('Total duration: {:%H:%M:%S}'.format(datetime_original))

    def reset_playlist(self):
        """Clears the playlist."""
        self.tracks.clear()
        print(f'{self.title} has been reset.')

    def is_empty(self):
        """Returns True if playlist is empty, False otherwise"""
        return len(self.tracks) == 0


Track Class

There is also a Track class associated with the project. This is the track.py file which contains the Track class.

class Track:
    """A Track class"""

    def __init__(self, title, artist, duration):
        """Initialize attributes."""
        self.title = title
        self.artist = artist
        self.duration = duration

    def __repr__(self):
        return f'{self.__class__.__name__}(title={self.title}, artist={self.artist}, duration={self.duration} seconds)'

main.py

This is the main.py file.


from mp3playlist import MP3PlayList, Track


def main():
    # Initialize Playlist:
    mp3_playlist = MP3PlayList('MyCoolMusic', 'music_playlist.txt')

    # Check if empty:
    print(mp3_playlist.is_empty())  # True

    # Load playlist from textfile:
    mp3_playlist.load_playlist()

    # Remove a track by title
    mp3_playlist.remove_track('one')

    # Add a new track:
    track = Track('metallica', 'the unforgiven', '6:23')
    mp3_playlist.add_track(track)

    # Informs user if file cannot be found:
    mp3_playlist.remove_track('bogus_file')  # File cannot be located!

    # Shuffles list 3 x times:
    mp3_playlist.shuffle_playlist(3)

    # Get total number of tracks:
    mp3_playlist.get_number_of_tracks()

    # Get total duration of playlist
    mp3_playlist.get_total_duration()

    # Save Playlist and filename by name designated at instance initialization
    mp3_playlist.save_playlist()

    # Shuffle list 7x times:
    mp3_playlist.shuffle_playlist(7)

    # Create a file with a new custom file_name:
    mp3_playlist.save_playlist('backup_copy.txt')


if __name__ == '__main__':
    main()

music_playlist.txt

The data from music_playlist.txt was used to build the Track class, which is then loaded into the MP3Playlist class.

cruel summer,bananarama,3:50
no ordinary love,sade,2:41
when doves cry,prince,3:45
jammin,bob marley,3:20
veridis quo,daft punk,5:58
hotel california,eagles,6:30
one,metallica,7:26
kunglim guli,yulduz usmanova,3:45
mea culpa,enigma,5:02
solitude,marion,4:11
tokyo,skeler,3:39
un simple histoire,thievery corporation,3:45
in the air tonight,phil collins,5:37
metallica,the unforgiven,6:23

The Project Directory Structure

This is the directory structure for the project. The text files used in this project are stored in music_file directory.


MP3Playlist/
├── music_files/
│   ├── music_playlist.txt
│   ├── backup_copy.txt
│   
├── mp3playlist.py  
├── track.py
├── main.py 

Here is a link to my MP3 Playlist project on GitHub.

Goals & Expectations

My goal is to be an expert level Python programmer someday. Therefore, tell me where I can improve my code. Any comment of constructive criticism are welcomed.

Questions

  1. Is the code organized, well documented, and easy to read? If not, where can I improve?
  2. Please analyze and critique my load_playlist(), save_playlist(), and remove_track() functions in particular. Are these functions written well? Could they be written better? Could you offer any code examples of you would have approached these functions?
  3. Are there any other areas of my code that could be re-written more efficiently?
  4. This is my first code review; is there a question I should have ask?
\$\endgroup\$
1
  • \$\begingroup\$ Excellent first question \$\endgroup\$
    – Reinderien
    Jul 7, 2022 at 11:47

2 Answers 2

5
\$\begingroup\$

Your code in my opinion is well written and well organized. But to make it even better I have a few suggestions:

Even more type annotations

Although you've used type annotations but at some places it can be used to make your code more expressive. For example in __init__ of MP3PlayList you can add type hint to tell the reader that self.tracks is a list of Track objects. Something like this:

    self.tracks: list[Track] = []

Using data classes

The Track class, AFAIK has no functionality. It just stores data about a special track. In this case you can use data classes. One of the main benefit of using them is that you no longer need to define __init__ or __repr__ by hand. Maybe the equivalent implementation of Track class using data classes is this:

>>> from dataclasses import dataclass
>>> 
>>> @dataclass
... class Track:
...     title: str
...     artist: str
...     duration: int
... 
>>> 
>>> 
>>> track = Track(title="Night Call", artist="London Grammar", duration=270)
>>> repr(track)
"Track(title='Night Call', artist='London Grammar', duration=270)"
>>> 
>>> track
Track(title='Night Call', artist='London Grammar', duration=270)
>>> 

Handling exceptions

If the music_playlist.txt is not organized as your will or the file was corrupted your program crashes. So I think it is better to use try statement in the load_playlist method and handle the condition that the file is not in the correct format.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thank you for your feedback! I appreciate you showing me how to implement type hint for self.tracks list. I didn't realize you could do that ~very cool! I updated changes to my GitHub. Also, thank you for bringing dataclasses to my attention; I have a lot to learn! \$\endgroup\$
    – Seraph776
    Jul 8, 2022 at 20:33
  • \$\begingroup\$ @Seraph776 I hope it helps you :) \$\endgroup\$ Jul 9, 2022 at 5:27
2
\$\begingroup\$

There's not much point to defining a __repr__ for these classes - that's intended for machine legibility, and you're more concerned with human-legible printing so should override __str__ instead. Along those lines, show() should be changed so that it doesn't print, and its contents returned in __str__.

All of your existing comments are less useful than having no comment at all. Things that should actually be commented include your file format.

load_playlist being an instance member that mutates an existing instance of the class is not a good idea. Instead, consider writing a @classmethod pseudoconstructor that loads the playlist into a new instance. Said in another way, why should we require that there be a useless, empty class instance before loading any file? It should be immediately populated.

Why should we force the use of the music_files prefix? This can be added at the outer level by the caller.

Prefer pathlib.Path instead of the os.path methods.

Don't parse CSV yourself. What if a song or artist title has a comma in it? Python has a csv module to do this for you safely.

Don't index into Track(song[0], song[1], song[2]) by integer. Do a positional tuple unpack instead.

None of the methods in your playlist class should print, as the class is a business logic class. Printing can occur at the outer level.

filename should probably not be kept as a property of the class, and can just be a parameter to the load and save methods.

remove_track can be simplified using a generator to which you apply either a next() or a tuple unpack of a single element, depending on whether you allow duplicates or not. It should raise an exception rather than printing an error.

I don't see the point in shuffling a playlist multiple times. In theory, with a good enough random generator, you'll be entirely unable to distinguish whether a playlist has been shuffled once or a million times.

Your methods for the number of tracks, total track duration and empty checking can be converted into property methods.

is_empty should be reused in your show() logic.

Consider including the title of the playlist in your show() logic.

Reliance on the current year and a full datetime for what should only be timedelta instances is pretty nasty. Stay in the timedelta domain.

Suggested

from datetime import timedelta
from pathlib import Path
from random import shuffle
from typing import NamedTuple, Iterable, Sequence, Iterator
import csv


class MP3PlayList:
    def __init__(self, title: str, tracks: list['Track']) -> None:
        self.title = title
        self.tracks = tracks

    @classmethod
    def load(cls, filename: Path, title: str) -> 'MP3PlayList':
        """
        Load the playlist from a header-less CSV with format looking like:
        artist name,track name,6:23
        The track duration is in totalminutes:seconds (no hours).
        """
        with filename.open() as file_obj:
            reader = csv.reader(file_obj)
            tracks = list(Track.from_rows(reader))
        return cls(title=title, tracks=tracks)

    def save(self, filename: Path) -> None:
        """
        Save the playlist to a header-less CSV with format looking like:
        artist name,track name,6:23
        The track duration is in totalminutes:seconds (no hours).
        """
        with filename.open('w') as file_obj:
            writer = csv.writer(file_obj)
            writer.writerows(Track.to_rows(self.tracks))

    def add_track(self, track: 'Track') -> None:
        self.tracks.insert(0, track)

    def remove_track(self, track_name: str) -> None:
        """
        Remove a track by name. The name must match exactly one track or a ValueError will be raised.
        """
        i_track, = (
            i for i, track in enumerate(self.tracks)
            if track.title == track_name
        )
        self.tracks.pop(i_track)

    def __str__(self) -> str:
        if self.is_empty:
            return f'Track list "{self.title}" is empty!'

        return (
            f'Track list "{self.title}":\n'
            + '\n'.join(
                f'\t• {track}' for track in self.tracks
            )
        )

    def shuffle(self) -> None:
        shuffle(self.tracks)

    @property
    def track_count(self) -> int:
        return len(self.tracks)

    @property
    def total_duration(self) -> timedelta:
        return sum(
            (track.duration for track in self.tracks),
            start=timedelta(),
        )

    def reset_playlist(self) -> None:
        self.tracks.clear()

    @property
    def is_empty(self) -> bool:
        return len(self.tracks) == 0


def parse_duration(s: str) -> timedelta:
    """
    Parse into a timedelta a string of form totalminutes:seconds
    """
    minutes, seconds = s.split(':')
    return timedelta(minutes=int(minutes), seconds=int(seconds))


def format_duration(delta: timedelta) -> str:
    """
    Format a timedelta to a string of form totalminutes:seconds
    """
    minutes, seconds = divmod(delta.total_seconds(), 60)
    return f'{minutes:.0f}:{seconds:02.0f}'


class Track(NamedTuple):
    title: str
    artist: str
    duration: timedelta

    @classmethod
    def from_rows(cls, rows: Iterable[Sequence[str]]) -> Iterator['Track']:
        for title, artist, dur_str in rows:
            yield cls(
                title=title, artist=artist, duration=parse_duration(dur_str),
            )

    @staticmethod
    def to_rows(tracks: Iterable['Track']) -> Iterator[tuple[str, ...]]:
        for track in tracks:
            yield track.title, track.artist, format_duration(track.duration)

    def __str__(self) -> str:
        return f'{self.title} - {self.artist} ({format_duration(self.duration)})'


def main() -> None:
    parent = Path('music_files')
    mp3_playlist = MP3PlayList.load(parent / 'music_playlist.txt', 'MyCoolMusic')
    print(mp3_playlist)

    mp3_playlist.remove_track(track_name='one')
    track = Track('metallica', 'the unforgiven', timedelta(minutes=6, seconds=23))
    mp3_playlist.add_track(track)

    try:
        mp3_playlist.remove_track('bogus_file')
    except ValueError:
        print('Track could not be located. Please try again.')

    mp3_playlist.shuffle()
    print(mp3_playlist.track_count)
    print(mp3_playlist.total_duration)

    mp3_playlist.shuffle()
    mp3_playlist.save(parent / 'backup_copy.txt')


if __name__ == '__main__':
    main()

Output

Track list "MyCoolMusic":
    • cruel summer - bananarama (3:50)
    • no ordinary love - sade (2:41)
    • when doves cry - prince (3:45)
    • jammin - bob marley (3:20)
    • veridis quo - daft punk (5:58)
    • hotel california - eagles (6:30)
    • one - metallica (7:26)
    • kunglim guli - yulduz usmanova (3:45)
    • mea culpa - enigma (5:02)
    • solitude - marion (4:11)
    • tokyo - skeler (3:39)
    • un simple histoire - thievery corporation (3:45)
    • in the air tonight - phil collins (5:37)
    • metallica - the unforgiven (6:23)
Track could not be located. Please try again.
14
1:04:49
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you so much for taking the time to post your code, It looks absolutely professional! This is the greatest expectation I could have hoped for in a code review. Thank you again for all of your feedback! \$\endgroup\$
    – Seraph776
    Jul 9, 2022 at 5:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.