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 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
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
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
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