MP3 Vote Collator - Final

Question

Here is the completed code for my first Python project, for which I received some help and feedback here: Collate votes on MP3 files to list them by popularity

Seeing as there have been considerable changes since that post, I figured it would be best to start a new post.

My goals when starting this project were:

• Scan sub folders for MP3 files (named in a predetermined format).
• Sum the total amount of votes per song and output the results to a CSV file.
• Create a list of who voted for which songs.
• Copy those MP3 files to a single sub directory and rename them based on the results. For example, if a song received a total of 18 votes and came in at 11th position, it would have '11.' prepended at the start. This allows the MP3 files to be imported straight into a playlist in the correct order for a countdown.

I have completed these goals and my tests so far have worked perfectly. I am not totally satisfied with the final function, process_mp3. It seems a bit long-winded and could be improved, so I am looking for some feedback on that in particular.

import os, csv, operator, itertools, shutil
from collections import Counter, defaultdict

print("*****Welcome to the MP3 Vote Collator*****" + '\n')

# Global variables and lists
votes = defaultdict(list)
tally = Counter()
folder_list = os.getcwd()
base_dir = os.getcwd()

# The tally_votes function is responsible for searching for all .mp3 files, retrieving the vote count for each song and
# storing the results in the 'tally' counter.

def tally_votes():
    print("STAGE 1. Processing Votes..." + '\n')
    ignore_dir = ["Playlist"]
    for subdir, dirs, files in os.walk(folder_list):
        dirs[:] = [d for d in dirs if d not in ignore_dir]
        for file in files:
            if file.endswith(".mp3"):
                file_name = file.split('.')[1] # Removes the .mp3 extension and numbers at the start of the file to clean up song name
                vote_count = file.split('.')[0] # Retrieves vote count for each song
                _, tail = os.path.split(subdir) # Retrieves the name of the voter from the folder name
                song = file_name.lower() # Lower-casing prevents song name match issues

                try:
                    tally[song] += int(vote_count) # Adds song to tally. If song already exists, adds votes to that song
                    votes[song].append((tail, int(vote_count))) # Adds voter name and vote count to votes dict
                except:
                    break

# The write_tally function will write the results to a CSV file called 'results_out.csv' in the base directory

def write_tally():
    tally_list = sorted(list(Counter((tally)).items()),key=operator.itemgetter(1), reverse=True) # Converts tally counter to a list and sorts songs based on highest votes
    votes_list = sorted(list((votes).items()),key=operator.itemgetter(0)) # Converts votes dict to a list and sorts in alphabetical order
    with open('results_out.csv', 'w') as resultsout, open('vote_list.csv', 'w') as votelistout:
        resultswriter = csv.writer(resultsout)
        votelistwriter = csv.writer(votelistout)
        resultswriter.writerows(tally_list)
        votelistwriter.writerows(votes_list)

# The process_result function will read the results from the CSV and print them to output

def process_result():
    print("STAGE 2. Listing Top 3 Songs:" + '\n')
    with open('results_out.csv', 'r') as results:
        reader = csv.reader(results)
        for iteration, row in enumerate(csv.reader(results)):
            print("The number {} song for this year's count is: {}, with a total of {} votes!".format(
                    ["one", "two", "three"][iteration], row[0], row[1]))
            if iteration >= 2:
                break
        print('\n' + "There are a total number of " + str(len(tally)) + " songs in this year's count.")

# The process_mp3 function will read the CSV file line by line, rename MP3 files accordingly and then copy them
# to the 'Playlist' directory

def process_mp3():
    print('\n' + "STAGE 3. Grouping and Renaming Mp3 Files")
    ignore_dir = ["Playlist"] # Ignore this dir to avoid issues when copying MP3 files(I.e. src/dst are the same). This dir must be empty prior to running script
    for subdir, dirs, files in os.walk(folder_list):
        dirs[:] = [d for d in dirs if d not in ignore_dir]
        for file in files:
            if file.endswith(".mp3"):
                file_name = file.split('.')[1]
                with open('results_out.csv', 'r') as results:
                    reader = csv.reader(results)
                    for iteration, row in enumerate(csv.reader(results)):
                        song_name = row[0].title() # Capitalize song names for cleaner look
                        mp3_file = os.path.join(os.path.join(subdir), file)
                        new_name = ("{}".format("%02d" % range(1, 1000, 1)[iteration]) + "." + song_name + ".mp3") # Rename MP3 files according to correct order
                        playlist_dir = ("Playlist")
                        if not os.path.exists(playlist_dir): # For storing renamed MP3 files
                            os.mkdir(playlist_dir)
                        if (file_name.lower() == song_name.lower() and
                            song_name not in playlist_dir):
                            shutil.copy(mp3_file, playlist_dir) # Currently copies for testing purposes. Probably best to change this to shutil.move
                            os.chdir(playlist_dir)
                            os.rename(file, new_name)
                            os.chdir(base_dir) # Return to base directory for restart of function

if __name__ == '__main__':
    tally_votes()
    write_tally()
    process_result()
    process_mp3()

Answer 1

Just some comments:

tally_votes() and process_mp3(), why is one singular and the other plural? Same for everywhere else where single/plural are inconsistent.

---

file_name = file.split('.')[1] - this assumes that the files match a naming pattern, but doesn't check that they do. If they don't, it will quietly do the wrong thing. Might not matter for your use case.

similarly, file.endswith(".mp3") might not catch if the filename is ".MP3".

---

folder_list and base_dir are the same thing? Why not use os.walk(base_dir) and get rid of folder_list? (which is weirdly named, since it's not a list).

---

ignore_dir = ["Playlist"]
dirs[:] = [d for d in dirs if d not in ignore_dir]

ignore_dir is a list, why aren't you calling it ignore_list or ignore_dirs (plural) like you have elsewhere? Inconsistent naming doesn't matter enormously, but it does make it easier for bugs to hide where you reference the wrong thing by mistake.

This is neat design for if you have more than one directory to ignore, but with just one you can shorten it to dirs.remove('Playlist')

You also have the playlist_dir repeated again in process_mp3(), this time as a plain string. So instead, make it a constant at the top, playlist_dir = "Playlist", use dirs.remove(playlist_dir) and os.path.exists(playlist_dir)

---

list(Counter((tally)).items()), - tally is already a Counter. And items() already returns a list, but the entire line:

tally_list = sorted(list(Counter((tally)).items()),key=operator.itemgetter(1), reverse=True) can it become tally_list = tally.most_common() ?

---

os.path.join(subdir) - joining a single thing does nothing. The same as just subdir.

---

new_name = ("{}".format("%02d" % range(1, 1000, 1)[iteration]) + "." + song_name + ".mp3") - is this just a way to add 1 to the iteration number before putting leading zeros? If so, either do iteration + 1 or use the fact that enumerate() can take a parameter to say what number it starts counting from.

If you must keep that format, why not put all of the song name in one string using format() instead of using format and then + "." + ...

---

You're walking the directory tree at the top of each function, but doing file.lower() several times in different places - leading to a risk that you will forget it once, and introduce bugs. If you need to clean up some kind of input, it's neater to do it all in one place, in one go, and then you don't need to do it anywhere else. e.g. you might do

for subdir, dirs, files in os.walk(folder_list):

    mp3_files = [f.lower() for f in files if f.lower().endswith('.mp3')]

    for file in mp3_files:

to get just the mp3 files, in lowercase.

In process_mp3() where you make the song_name into titlecase then convert it back to lowercase to check if it matches the file name - that's the reverse of this idea. Do all the processing first, while it's still in lowercase, and convert it to titlecase for neat display right at the last moment before displaying it, so all the display/presentation/output code is grouped together, and isn't mixed with the processing code.

---

The overall design involves walking through all the files once in tally_votes(), and then again in process_mp3() - meaning they both start with the same chunk of code - but then in process_mp3() you reopen the results CSV, reread all through it and check for file_name.lower() == song_name.lower() for every single file in the walk. Even if a computer can do it quickly, that's really inefficient design.

If you changed the design a bit so that tally_votes() included storing the path to the files somewhere, and that was stored in the results CSV, then you'd get a much much simpler process_mp3() function. All it would have to do is read the CSV and rename the files, one file per CSV row.

No need for os.walk(), or filename splitting. You can put the os.path.exists test earlier in the function, no need to do it for every row in the CSV, and you needn't do as much os.chdir because you'll always be working with the full path to each file - and know where the full path to the os.path.join(base_dir, playlist_dir) is.

Alternately, if you don't want to put the full path to each song in the CSV and read it back, you could change the design to bring the os.walk code out into a separate function:

def get_mp3_files():
    mp3_files = []
    for subdir, dirs, files in os.walk(base_dir):
        ...
        mp3_files.append('/another/file.mp3')

    return mp3_files

def tally_votes():
    for file in get_mp3_files():
        ...

def process_mp3():
    for file in get_mp3_files():
        ...

and that would condense the two sets of code into one place, and make it easier to update without missing anything in one or the other.

Answer 2

Docstring

Describing a function should not go over it as a comment but after it as a docstring:

def tally_votes():
   """
   The tally_votes function is responsible for searching for all .mp3 files,
   retrieving the vote count for each song and
   storing the results in the 'tally' counter.
   """

This way the user will read it when issuing help(tally_votes)

Return values and arguments

In short do not write:

def f():
   return x + 1

but:

def f(x):
   return x + 1

That is, give parameters to your functions.

For example process_result() should get the filename as an argument.

Constants

What if you decide that you know store music in ".waw" format? Looking for all the occurrences of ".mp3" and replacing them is sub-optimal.

So at the top of your file write

MUSIC_EXTENSION = ".mp3"

To simplify future changes.

Let the names tell, not the comments

Maintaining and upgrading both code and comments is double the effort than just comments.

For example:

            _, tail = os.path.split(subdir) # Retrieves the name of the voter from the folder name 

Becomes:

_, voter_name = os.path.split(subdir)

Given the more specific name, the comment is not needed anymore.

Tuple unpacking + avoiding temporaries

            file_name = file.split('.')[1] # Removes the .mp3 extension and numbers at the start of the file to clean up song name
            vote_count = file.split('.')[0] # Retrieves vote count for each song
            song = file_name.lower() # Lower-casing prevents song name match issues

Becomes:

vote_count, song = file.lower().split('.')

Much cleaner.

Avoid except:

Always specify the exception when you use except.

Answer 3

Your handling of tally and votes is not particularly recommended, as you are waiting for an accident regarding global variables to happen. Try the following:

votes = {}

def sketchy_tally_votes():
    #global votes
    votes['works'] = 1203
    votes = { 'fails' : 123}

def print_votes():
    print votes

if __name__ == '__main__':
   sketch_tally_votes()
   print_votes()

By a lucky break your code is working as you use votes[song] and as thus doesn't change the global variable, you only dereference it. The same applies to the tally variable.

The proper way of handling this would be either to:

1. Add global votes and global tally at start of tally_votes, and similar for other variables that you want to use from a global perspective but within a function (aka uncomment the global votes in the code above
2. By transferring the lists to and from your functions as suggested in my previous answer on previous version of this question.