1
\$\begingroup\$

Part of my code:

def get_file():

    custom_data_folder = input("Name of custom data directory - " +
                               "Hit ENTER to default: ")
    file_name = input("File name: ")
    default_path = os.path.abspath(os.path.join('data', file_name))
    custom_path = os.path.abspath(os.path.join(custom_data_folder, file_name))

    if custom_data_folder == '':
        return(default_path if os.path.exists(default_path) is True 
                         else "The file or path does not exist.")
    else:
        return(custom_path if os.path.exists(custom_path) is True 
                           else "The file or path does not exist.")

My script assumes a file is in a directory called "data", but I wanted to provide an option of a entering a custom directory where a file could be.

It works, but is there a better way to go about doing what this function is trying to accomplish?

\$\endgroup\$
3
\$\begingroup\$

You can simplify several things:

  • if custom_data_folder == '': can become if not custom_data_folder:
  • if os.path.exists(default_path) is True can be just if os.path.exists(default_path)

Other notes:

  • it is questionable that you return "The file or path does not exist." string in case a path does not exist. Throwing an error would probably make more sense
  • make your default directory name a constant
  • you can avoid calculating the full default path if custom is provided
  • you can reduce code duplication and construct a path first, then check if it exist, raise an error if it does not and return if does exist

All the above things taken into account, here is the modified code:

import os


DEFAULT_DIRECTORY = 'data'


def get_file(): 
    """TODO: docstring"""
    custom_data_directory = input("Name of custom data directory - Hit ENTER to default: ")
    file_name = input("File name: ")

    directory = custom_data_directory or DEFAULT_DIRECTORY
    path = os.path.abspath(os.path.join(directory, file_name))

    if not os.path.exists(path):
        raise ValueError("Path '{path}' does not exist".format(path=path))

    return path

Some thoughts for further improvements:

  • you should probably create a better separation of concerns and have this function parameterized with file and directory names - moving user input related things from out of the function
  • create a docstring explaining what your function does
  • .format() formatting can be replaced with an f-string if on Python 3.6+
  • get_file is probably not the most descriptive name for this function, consider renaming it - get_full_file_path?..
\$\endgroup\$
2
  • \$\begingroup\$ Interesting advice, thank you. In regards to your "separation of concerns" point, I agree that they should be shifted out of the function. After reading your answer, I think perhaps argparse could help in that. Thank you for all the advice. Your answer has given me much to work with. \$\endgroup\$ – Walid Mujahid وليد مجاهد Aug 1 '17 at 18:19
  • \$\begingroup\$ @WalidMujahid ah, yes, argparse is a good idea - you can even use custom file and directory argument types there. Thanks. \$\endgroup\$ – alecxe Aug 1 '17 at 18:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.