5
\$\begingroup\$

This is the first program I've created without help from a tutorial. Written in Python 3.4, it gathers a specified number of media files and opens them in the default player (I use VLC on Windows 8.1). Python is my first language I have been learning mostly through trial and error. I am posting here to see what I can improve and to learn as much as I can.

import os, random, sys

completed_media = (r"C:\Users\user_000\Desktop\Completed Media")

all_media = []
playlist_ = []

def create_media_list():
    for path, subdirs, files in os.walk(completed_media):
        for file in files:
            if file.lower().endswith(('.mkv', '.mp4', '.divx', '.avi')):
                all_media.append(os.path.join(path, file))

def random_selection_from_media():
    random_selection = random.choice(all_media)
    if random_selection not in playlist_:
        playlist_.append(random_selection)
    else:
        pass

def get_selections():
    for i in range(number):
        random_selection_from_media()
    print_playlist_()
    playlist_confirmation()

def number_of_selections():
    while True:
        try:
            global number
            number = int(input('How many files would you like to add to playlist? >>> '))
            break
        except ValueError:
            print('Enter a number.')

def print_playlist_(): 
    print('\n-------In playlist-------\n')
    print('[{0}]'.format('\n-------------------------\n'.join(str(i) for i in enumerate(playlist_, 1))))
    print('\n-------End playlist------\n')

def remove_selection():
    while True:
        try:
            to_remove = int(input('To remove a selection enter the number of the selection you want removed here. >>> '))
            if to_remove <= len(playlist_):
            break
        except ValueError:
            print('Enter a number.')
            remove_selection()
        try:
            playlist_.pop((to_remove - 1))
            break
        except (IndexError, UnboundLocalError):
            print('Enter a vaild number')
            remove_selection()
    clear()
    print_playlist_()
    playlist_confirmation()

def playlist_confirmation():
    ok = input('This list ok? >>> ').lower()
    if ok == 'yes' or ok == 'y':
        play_playlist_()
    elif ok == 'no' or ok == 'n':
        while True:
            new = input('Get new list or remove a specific selection? >>> ').lower()
            if new == 'new list' or new == 'n':
                del playlist_[:]
                clear()
                get_selections()
                break
            elif new == 'specific selection' or new == 's':
                remove_selection()
                break
            else:
                 print('Enter \'new\' or \'selection\'')
    else:
        playlist_confirmation()

def play_playlist_():
    for i in playlist_:
        play_cmd = "rundll32 url.dll,FileProtocolHandler \"" + i + "\""
        os.system(play_cmd)

def clear():
    os.system('cls')

def main():
    create_media_list()
    number_of_selections()
    get_selections()

if __name__=="__main__":
    main()
\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

Pretty good for a self-taught beginner, keep going like this and you're golden!

Recursion and infinite loop

This is probably the worst part of the script:

def remove_selection():
    while True:
        try:
            to_remove = int(input('To remove a selection enter the number of the selection you want removed here. >>> '))
            if to_remove <= len(playlist_):
                break
        except ValueError:
            print('Enter a number.')
            remove_selection()
        try:
            playlist_.pop((to_remove - 1))
            break
        except (IndexError, UnboundLocalError):
            print('Enter a vaild number')
            remove_selection()

An infinite loop, a misused exception handling, an incorrect error handling, and recursive calls... This is confusing and more complicated than it needs to be.

You can replace the recursive calls with continue.

Exception handling for converting the input to integer is appropriate, because there is no better way. It is not appropriate for validating the input is within range, because a better way exists using conditionals.

Also, it's not enough to check that the index is less than the length of the list, you also need to check that it's 0 or greater.

UnboundLocalError is thrown when trying to access a variable that hasn't been assigned. That's not an exception to catch, it indicates a problem in the logic that needs to be fixed.

Here's one way to address these issues:

while True:
    try:
        to_remove = int(input('To remove a selection enter the number of the selection you want removed here. >>> '))
    except ValueError:
        print('Enter a number.')
        continue
    if not (0 < to_remove <= len(playlist_)):
        print('Enter a valid number (within range: 1..{})'.format(len(playlist_)))
        continue
    playlist_.pop(to_remove - 1)
    break

Global variables

Try to avoid global variables as much as possible.

For example, instead of this:

def number_of_selections():
    while True:
        try:
            global number
            number = int(input('How many files would you like to add to playlist? >>> '))
            break
        except ValueError:
            print('Enter a number.')

It would be better to make the function return the number:

def number_of_selections():
    while True:
        try:
            return int(input('How many files would you like to add to playlist? >>> '))
        except ValueError:
            print('Enter a number.')

And then adjust the callers appropriately to pass the number as parameter where needed, for example instead of:

number_of_selections()
get_selections()

Write:

get_selections(number_of_selections())

Adjust the rest of the code accordingly, and try to eliminate the other global variables too, all_media and playlist_. For these two, another good alternative will be to create a class, where these variables will be member fields.

Simplifications

Instead of this:

if ok == 'yes' or ok == 'y':

A bit simpler way to write:

if ok in ('yes', 'y'):

Optimizing imports, and coding style

The script is not actually using sys, so you can drop that import.

And the style guide recommends to import one package per line, like this:

import os
import random

Do read the style guide, it has many other recommendations that apply to your script.

\$\endgroup\$
1
  • \$\begingroup\$ I have edited my question with an attempt at creating a class to replace the global variables. It works but I'd very much appreciate you having a look at it, just to verify that its acceptable use of class. Also, thank you so much for your review I have learned a lot from it! \$\endgroup\$ Commented May 18, 2016 at 18:24

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.