Skip to main content
Code Review

Return to Answer

Add some more suggestions.
Source Link
Booboo
Booboo
  • 3.7k
  • 4
  • 15

Remove Unnecessary Open Check

The latest documentation for the built-in open is a bit inconsistent. It states:

Open file and return a corresponding file object. If the file cannot be opened, an OSError is raised.

But later on in the documentation it states that as of release 3.3:

FileExistsError is now raised if the file opened in exclusive creation mode ('x') already exists.

And we know that is you are opening a file for reading and the file does not exist, you get a FileNotFoundError.

But in the end, if the file cannot be successfully opened, an exception is raised. Testing for file.closed is something you might want to do for a previously successfully opened file to see if it has been subsequently closed. So you can remove this check. Also, as of Python 3.3:

IOError used to be raised, it is now an alias of OSError.

So, assuming you want to return the open file, you might code the function as:

def open_csv_file():
    """
    Display the prompt for the user to specify the path for the CSV file that they wish to open in the program.
    
    Returns
        The opened CSV file
    """
    # Infinite loop to open the CSV file, if file not found then it will loop again until the file is found.
    while True:
        menu.print_menu_prompt()

        timesheet_csv = input("\nPlease specify the CSV path to be opened: ")

        # Try and except to check if the CSV file has opened successfully, if so then it is returned to main.
        try:
            file = open(timesheet_csv, 'r', newline='\n')
        except FileNotFoundError:
            print(f"\nERROR: The specified file does not exist.")
        except IOError:
            print("\nAn error occured while trying to open the CSV file.")
        else:
            print("\nThe CSV file opened successfully")
            return file

But how does the user escape this function if they cannot specify the path of a CSV file? See the next section!

Separation of Concerns

You should have one function that takes as an argument a path to a CSV file and whose sole concern is the opening of that file. The function either returns the opened file or raises an exception if the file cannot be opened. Then a second function would be concerned with prompting the user for the path. If the user specifies an empty string, you would "escape" this function. Otherwise, the path would be passed to the first function.

Remove Unnecessary Open Check

The latest documentation for the built-in open is a bit inconsistent. It states:

Open file and return a corresponding file object. If the file cannot be opened, an OSError is raised.

But later on in the documentation it states that as of release 3.3:

FileExistsError is now raised if the file opened in exclusive creation mode ('x') already exists.

And we know that is you are opening a file for reading and the file does not exist, you get a FileNotFoundError.

But in the end, if the file cannot be successfully opened, an exception is raised. Testing for file.closed is something you might want to do for a previously successfully opened file to see if it has been subsequently closed. So you can remove this check. Also, as of Python 3.3:

IOError used to be raised, it is now an alias of OSError.

So, assuming you want to return the open file, you might code the function as:

def open_csv_file():
    """
    Display the prompt for the user to specify the path for the CSV file that they wish to open in the program.
    
    Returns
        The opened CSV file
    """
    # Infinite loop to open the CSV file, if file not found then it will loop again until the file is found.
    while True:
        menu.print_menu_prompt()

        timesheet_csv = input("\nPlease specify the CSV path to be opened: ")

        # Try and except to check if the CSV file has opened successfully, if so then it is returned to main.
        try:
            file = open(timesheet_csv, 'r', newline='\n')
        except FileNotFoundError:
            print(f"\nERROR: The specified file does not exist.")
        except IOError:
            print("\nAn error occured while trying to open the CSV file.")
        else:
            print("\nThe CSV file opened successfully")
            return file

But how does the user escape this function if they cannot specify the path of a CSV file? See the next section!

Separation of Concerns

You should have one function that takes as an argument a path to a CSV file and whose sole concern is the opening of that file. The function either returns the opened file or raises an exception if the file cannot be opened. Then a second function would be concerned with prompting the user for the path. If the user specifies an empty string, you would "escape" this function. Otherwise, the path would be passed to the first function.

Source Link
Booboo
Booboo
  • 3.7k
  • 4
  • 15

I will not repeat what others have so far said.

Open the File Correctly!

@Chris commented that "Nothing about this seems specific to CSV files."

I would go one step further and tell you that the way you are opening files will not always work if its intended use of the file is with the csv module. If you look at the documentation for the csv module, specifically the footnote at the bottom of the page, you will see:

If newline='' is not specified, newlines embedded inside quoted fields will not be interpreted correctly, and on platforms that use \r\n line endings on write an extra \r will be added. It should always be safe to specify newline='', since the csv module does its own (universal) newline handling.

But this requirement is not specified only in a footnote. If you look at the description for csv.reader on this page you will see:

If csvfile is a file object, it should be opened with newline=''.

And the following example is provided:

import csv
with open('eggs.csv', newline='') as csvfile:
    spamreader = csv.reader(csvfile, delimiter=' ', quotechar='|')
    for row in spamreader:
        print(', '.join(row))

All csv functions that take as an argument an open file should ensure the file is opened specifying newline='' to work correctly for all platforms and CSV contents.