Skip to main content
Code Review

Return to Answer

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

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
Source Link
tsleyson
  • 1k
  • 5
  • 18

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

AltStyle によって変換されたページ (->オリジナル) /