Renaming PDF files based on given rules in a text file

I'm learing Python by writing a program to solve some housekeeping I do weekly, namely renaming PDF files according to rules given in a text file.

My end goal is to have idiomatic Python, with full test coverage. I have done some refactoring, and written a couple of tests. Ideally I would have done TDD from the beginning, but I find it challenging to write tests since I don't know Python that well yet.

The PDF documents can be named with two patterns:

xx.pdf
xx-y.pdf


They are stored in the folder ./test relative to the script root.

The text file Instruments.txt contains the following pattern:

01 SopranoCn
02 SoloCn
03 RepianoCn
...
18 Percussion
20 AllParts
...


The whole purpose is to rename files like this:

xx.pdf (01.pdf) --> xx_{instrument_name}.pdf (01_SopranoCn.pdf)
xx-y.pdf (02-1.pdf) --> xx_{instrument_name}{part_number}.pdf (02_SoloCn1.pdf)


The y-argument is optional, and can be added to any instrument.

Here's the whole source:

`"""
Invoke with "python notesort.py ".
A list of instrument numbers with corresponding instrument names
must be available in the root folder of the script.
"""

# Standard library imports
import os

FOLDER = './test'

def create_instrument_map():
"""Create a dictionary mapping each prefix
to the corresponding instrument name."""
with open('Instruments.txt') as instrument_file:
return {
line.split()[0]: line.split()[1] for line in instrument_file
}

def rename_files_with_instrument_name(instrument_map):
"""Rename PDF files in folder based on the root of
the original file name."""
for filename in os.listdir(FOLDER):
root, extension = os.path.splitext(filename)
if not extension.endswith('.pdf'):
continue
try:
if root[2]:
if root[:2] in instrument_map:
new_file_name = '{prefix}_'\
'{instrument_name}'\
'{part_n

For now I'll just throw up a few random thoughts.

The first thing I would do with rename_files_with_instrument_name is get rid of its reliance on the global FOLDER variable. Take a directory path as an argument, or shove it in a class and make the folder a parameter to the class's __init__ method which gets stored in an instance variable. That should immediately make it easier to write tests for rename_files_with_instrument_name; just pass in a folder and check that you get the results you expect.

The second thing I would do with rename_files_with_instrument_name is try and shorten the name a little. It's very long. Something like add_instrument can be good enough in context, but even rename_with_instrument is an improvement.

Going inside rename_files_with_instrument_name, using bald except is usually a bad practice. I gave an example case where bald except is bad in my answer to another question. If you think an exception might be thrown, it's better to name the specific exception. Also, make sure the situation where the exception gets raised is actually exceptional (an error or some situation which some code outside the function should handle). It looks a little like you're handling a normal, expected case in your except block—the case where the root filename is shorter than three characters. It's better to handle those with if. Not only is it easier to understand, it's also actually faster; catching exceptions is fairly time-consuming in most languages.

However, you don't actually need a conditional here; if you try to take a slice of a string which is longer than the entire string, the entire string just gets returned. That is:

>>> s = "ab"
>>> s[:30]
"ab"

So you can just write, without the try block, if root[:2] in instrument_map.

It's also usually good to avoid continue if you can. Sometimes you can't, and in those cases, don't contort yourself. But I think you can here. It looks like you could just write:

if extension.endswith(".pdf"):
    # All the code to construct a new filename

StackExchange Code Review Q#91721, answer score: 4