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()
janos
113k15 gold badges154 silver badges396 bronze badges
asked May 15, 2016 at 20:30
\$\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.

answered May 16, 2016 at 7:18
\$\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

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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.