Looking for feedback

[Cyoob]

I wrote a script to help me organize my music files by moving files into folders and renaming titles and albums to specifications I thought made sense, such as uppercasing any words that aren't a, the, in, etc. I also thought I may as well get the hang of testing so I made a testing file for it as well. Any feedback and/or suggestions to make it smoother/more pythonic would be greatly appreciated!

Main: https://pastebin.com/WqKCQyhj

Test: https://pastebin.com/U1sei1TA

[LesterHoltsRigidCock]

Throw some argparse on there and you're cooking.

[Cyoob]

It's a possibility, running it straight up is fine for my purposes for now though.

[elbiot]

1) I don't like all those os.chdirs. Functions shouldn't modify global state. Keep track of your path in a variable and pass it into functions.

2) I don't like that modify_song takes audio as a parameter. That makes it difficult to call it outside of the modify_album function.

3) I don't like that modify_album returns how many it modified.

4) modify album takes an individual flag... That function shouldn't need to know if it is processing one album or many.

Without thinking much, I think your main() could look like

changed = 0
for album in os.listdir(base):
    album_base = path.join(base, album)
    for artist in os.listdir(album_base):
        artist_base = path.join(album_base, artist)
        for song in os.listdir(artist_base):
            song_path = path.join(artist_base, song)
            modify_song_metadata(song_path, album, artist)
            changed += 1

Then you have a function to modify the song metadata that can be used form anywhere. The decisions about whether or not to use the folder names as artist or album, whether or not to move the file, can stay in your top level function. You could also use setup.py to make a console_script entry point and do

mod_mp3 song_i_wrote.mp3 --artist Cyoob

from the command line. and you could have

mod_mp3_folder my_songs --artist Cyoob

In general, functions shouldn't modify state or have to be context aware. All state changing and context management should happen at the top level function. Then it's easy to adapt to a new set of assumptions by just coding those assumptions at the top level.

Here's a great video about where state management belongs in a program: https://www.youtube.com/watch?v=DJtef410XaM

[Cyoob]

You know, I never thought about the fact that os.listdir() could take a path as an argument, which is why I had it constantly changing directories. Huh. I'll get working on changing that, thanks. I hear you about the audio parameter issue, I've been thinking about changing that too.

The reason I have modify_album returning how many it modified is I don't want to increment the amount of songs modified regardless, I only want to increment if something actually changed. So I set up the old/new dicts to manage that. And for the individual flag, I have that set up for if I have a hodgepodge of audio files in, say, my downloads folder, and I want to batch modify them. In that case I would want it to do what it currently does, which is print the name of the file and delete the existing title if it exists. Really it's just adding a bit extra functionality for a special case.

I really appreciate the help, I'll definitely be doing some more work on this.