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()
1 Answer 1
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.
-
\$\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\$user_000– user_0002016年05月18日 18:24:05 +00:00Commented May 18, 2016 at 18:24