5
\$\begingroup\$

So, I am either a glutton for punishment, or I have no life, but I wrote a script to answer a question over on Ask Ubuntu as a solution for selecting a random MP3 file in a given directory, and playing it in the specified player of choice. I am half-embarrassed to share this because I know it's not my best work and relies on user input for pretty much anything.

The script is designed to be passed the executable name for the media player the user wants to use, and select a random MP3 file to play in that player. There is a small level of Evil in here, because the person executing this script must specify a media player. It also assumes it's in the user's /home/$USER/Music directory on Linux-based systems, so to use another dir it has to be specified.

Any improvements you want to give me, feel free. I know this works, though. Main concerns are if I derped anywhere and did a bad thing.


randommp3.py (or, in a Gist if you want pretty):

#!/usr/bin/python

import getpass
import subprocess as sp
import os
import glob
import random
import argparse

if __name__ == "__main__":
    # Parse arguments to the script
    argparser = argparse.ArgumentParser(description="Open a random MP3 in the player of choice, or the default player",
                                        add_help=True)
    argparser.add_argument('--dir', '--directory', '--music-dir', dest='dirpath', type=str,
                           default=str('/home/' + getpass.getuser() + '/Music'), required=False,
                           help="The path to the directory where your music is stored. If the path has spaces, wrap the "
                                "entire path in single-quotes ('/home/ubuntu/My Music/' for example). If not specified, "
                                "the current user's Music directory in their /home/ folder is used.")
    argparser.add_argument('player', type=str, help="The executable name of the media player "
                                                    "to open the MP3 with. If none specified, "
                                                    "uses the system default player.")

    # Using the above 'argparser' items, get the arguments for what we're going to be using.
    args = argparser.parse_args()

    # Gp to the directory your music is in.
    os.chdir(args.dirpath)
    mp3s = glob.glob('*.mp3')

    # Modify the directory path to make sure we have the trailing slash
    dirpath = args.dirpath
    if dirpath[-1] not in '/\\':
        dirpath += '/'

    # Actually open the MP3 file, and /dev/null to suppress output messages from the process.
    DEV_NULL = open(os.devnull, 'w')
    execpath = [args.player, '%s%s' % (dirpath, str(random.choice(mp3s)))]
    sp.Popen(execpath, stdout=DEV_NULL, stderr=DEV_NULL)
\$\endgroup\$
4
\$\begingroup\$

I would improve the following things:

  • move the parsing argument part into a separate function (extract method)
  • define descriptions and helps in multiline string constants
  • use os.path.join() to join the path components - would also help to avoid worrying about a trailing slash
  • order imports alphabetically (PEP8 reference)

The improved code:

#!/usr/bin/python
import argparse
import getpass
import glob
import os
import random
import subprocess as sp


DESCRIPTION = "Open a random MP3 in the player of choice, or the default player"
DIRECTORY_ARGUMENT_HELP = """
The path to the directory where your music is stored. 
If the path has spaces, wrap the entire path in single-quotes ('/home/ubuntu/My Music/' for example). 
If not specified, the current user's Music directory in their /home/ folder is used.
"""
PLAYER_ARGUMENT_HELP = """
The executable name of the media player to open the MP3 with. 
If none specified, uses the system default player.
"""


def parse_args():
    """Parses command-line arguments and returns argparse arguments."""
    user_home_directory = os.path.join('/home', getpass.getuser())
    default_music_directory = os.path.join(user_home_directory, 'Music')

    parser = argparse.ArgumentParser(description=DESCRIPTION, add_help=True)
    parser.add_argument('--dir', '--directory', '--music-dir',
                        dest='dirpath', type=str, default=default_music_directory, required=False,
                        help=DIRECTORY_ARGUMENT_HELP)
    parser.add_argument('player', type=str, help=PLAYER_ARGUMENT_HELP)
    return parser.parse_args()


if __name__ == "__main__":
    args = parse_args()

    # Go to the directory your music is in.
    os.chdir(args.dirpath)
    mp3s = glob.glob('*.mp3')

    # Actually open the MP3 file, and /dev/null to suppress output messages from the process.
    DEV_NULL = open(os.devnull, 'w')
    execpath = [args.player, os.path.join(args.dirpath, random.choice(mp3s))]
    sp.Popen(execpath, stdout=DEV_NULL, stderr=DEV_NULL)

Note that if you would have been on Python 3.x, the DEVNULL could have been imported from the subprocess module:

from subprocess import DEVNULL 
\$\endgroup\$
  • 1
    \$\begingroup\$ Huh, I expected much worse in terms of punishment for failing to do code right heh. Thanks for your recommendations! \$\endgroup\$ – Thomas Ward Mar 3 '17 at 3:01
  • 1
    \$\begingroup\$ @ThomasWard yeah, the code overall is quite clean and understandable. I was also considering to post this note: No explicit error handling (e.g. what if the media player is not found, not executable etc).. \$\endgroup\$ – alecxe Mar 3 '17 at 3:08
  • \$\begingroup\$ @ThomasWard and, you know, there is always something to improve. I think I have about the same number of suggestions for the code posted in the answer :D I have no life too. \$\endgroup\$ – alecxe Mar 3 '17 at 3:12
  • \$\begingroup\$ Well then I would want it to raise an error heh. Hence no explicit handling. Lack of try/except/check_call was intentional. \$\endgroup\$ – Thomas Ward Mar 3 '17 at 3: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.