6
\$\begingroup\$

I am currently writing a program that allows me to search for files in a user-specified directory. My current code is as follows:

if os.path.exists(file_path)!= True:
    print('\n******* Path does not exist. *******\n')       

else:
    while True:
        aa = '''\nWhich search characteristics would you like to use? \n 1. Search by name: N \n 2. Search by extension: E \n 3. Search by size: S
            \n    Please enter your choice: '''

        answer = input(aa).strip()

        if answer not in 'NnEeSs' or answer=='':
            print('\n***** Invalid choice. *****')

        elif answer in 'Nn':
            while True:
                try:
                    name = input ('\nEnter file name: ')
                    rr = search_by_name(name, file_path)
                    if not rr:
                        print('\n***** File not found *****\n')
                    else:
                        break

                except WindowsError:
                    print('\n***** Oops! Access denied.*****\n')
                    continue


        elif answer in 'Ee':
            while True:
                try:
                    ending = input ('\nEnter the file extension: ')
                    rr = search_by_extention(ending, file_path)
                    if not rr:
                        print('\n***** No File(s) found *****\n')
                    else:
                        break

                except WindowsError:
                    print('\n***** Oops! Access denied. *****\n')
                    continue              

        elif answer in 'Ss':
            while True:
                try:
                    size = int(input('\nPlease enter file size: '))
                    rr = search_by_size(size, file_path)
                    if not rr:
                        print('\n***** No file(s) found *****\n')
                    else:
                        break

                except ValueError:
                    print('\n***** Enter an numeric value. *****\n')
                    continue

                except WindowsError:
                    print('\n***** Oops! Access denied. *****\n')
                    continue

Each of the search functions returns a list containing the result.

The try and except statements are concerning, and I feel they can be improved.

In each of these cases, or generally, how can the code be improved, or simplified?

\$\endgroup\$
2
\$\begingroup\$

To improve readability I would try and break the code down into more functions and redefine your pre-determined text values as globals, such that you can save space in the actual function. Responses can be represented by a dictionary value with boolean/text key value pairs (or exception/text). Also certain UI options can be stored in a dictionary, with links to their associated functions.

# using 3 quotes mean you don't need to specify newlines
dir_tools_menu = '''
Which search characteristics would you like to use?
1. Search by name: N 
2. Search by extension: E 
3. Search by size: S
\tPlease enter your choice: '''
dir_tools_options = {"n":ui_search_by_name, 
                     "e":ui_search_by_extention, 
                     "s":ui_search_by_size}
error_response = {WindowsError:"Oops! Access denied.", 
                  ValueError:"Enter an numeric value."}
# add number of files for true
files_found_response = {False:"No file(s) found", 
                        True:"{} file(s) found"}
# add filename
file_found_response = {False:"File {} not found",
                       True:"File {} found"}
file_exists_response ={ False:"Path does not exist.", 
                         True:"Path Found"}
# add choice for true
valid_ui_response = {False:"Invalid choice.", 
                      True:"You choose {}."}

Now that we have a collection of text values lets make a function to standardize print outputs, such that you don't have to write all those stars everywhere, and you can feel assured that they will produce even length lines.

line_length = 40
# or you can have a funky pattern like "`-._.-'-._...
line_pad = "*"*line_length 

def my_print(text, *params):
    # len(params) must match the count of "{}" in the text so this will not crash
    # Perhaps add a check to assert this case
    message = text.format(*params)
    offset = line_length/2 - len(message)/2
    if offset > 0 and offset + len(message) < line_length:
        print(line_pad[:offset] + message + line_pad[offset+len(message):])
    # in the case that a line is larger than the formatting, you can just print the line.
    # its gonna look gross regardless
    else:
        print(message)

Lets also make a simple yes/no function.

yes = ("yes", "y","1")
no = ("no", "n", "0")
def ask_yes_no(question):
    answer = ''
    while not answer in yes + no:
        answer = input(question).lower()
        if not answer in yes + no:
            my_print(valid_ui_response[False])
        else:
            my_print(valid_ui_response[True], return_to_menu)
            # True - yes, False - no
            return answer in yes

Now your main method boils down to..

quit_question = "Quit Dir Tools? "

def dir_tools(file_path):
    if os.path.exists(file_path):
        my_print(file_exists_response[True] )
    else:
        my_print(file_exists_response[False])
        return
    quit_dir_tools = False
    while not quit_dir_tools:
        answer = input(dir_tools_menu).lower()
        if answer not in dir_tools_options:
            my_print(valid_ui_response[False])
        else:
            my_print(valid_ui_response[True],answer)
            dir_tools_options[answer](file_path)
        quit_dir_tools = ask_yes_no(quit_question)

Now for the actual important functions.. I'll just do one example to show the try catch responses,..

ui_sbn_question = 'Enter file name: '
return_question = "Return to main menu? "

def ui_search_by_name(file_path):
    return_to_menu = False
    while not return_to_menu:
        name = input(ui_sbn_question)
        try:
            sbn_response = search_by_name(name, file_path)
            if not sbn_response:
                my_print(file_found_response[False], name)
            else:
                my_print(file_found_response[True], name)
                # assuming that this isnt a boolean
                my_print(sbn_response)
        except WindowsError:
            my_print(error_response[WindowsError])
        return_to_menu = ask_yes_no(return_question)
\$\endgroup\$
1
\$\begingroup\$

Overall, you need to look into PEP 8. Other then that, you need to apply a strategy pattern to get rid of redundant if/else loop. See more detailed analysis below.

Line by line analysis

if os.path.exists(file_path)!= True:

Use if not var.

    print('\n******* Path does not exist. *******\n')       

You have too many new lines in your code.

else:
    while True:
        aa = '''\nWhich search characteristics would you like to use? \n 1. Search by name: N \n 2. Search by extension: E \n 3. Search by size: S
            \n    Please enter your choice: '''

Wrap your code in 80 columns. Also, what kind of name is aa? Pick a meaningful name.

        answer = input(aa).strip()

        if answer not in 'NnEeSs' or answer=='':

if answer.lower() not in ('n', 'e', 's') is more readable. You don't need to check if it's empty either.

            print('\n***** Invalid choice. *****')

        elif answer in 'Nn':
            while True:
                try:
                    name = input ('\nEnter file name: ')

No space after input.

                    rr = search_by_name(name, file_path)

Pick a better variable name.

                    if not rr:
                        print('\n***** File not found *****\n')
                    else:
                        break

                except WindowsError:
                    print('\n***** Oops! Access denied.*****\n')
                    continue

No need for continue here.

        elif answer in 'Ee':
            while True:
                try:
                    ending = input ('\nEnter the file extension: ')
                    rr = search_by_extention(ending, file_path)
                    if not rr:
                        print('\n***** No File(s) found *****\n')
                    else:
                        break

                except WindowsError:
                    print('\n***** Oops! Access denied. *****\n')
                    continue              

        elif answer in 'Ss':
            while True:
                try:
                    size = int(input('\nPlease enter file size: '))

ValueError should be caught here, not down the road. Make your exception catches as narrow as you can.

                    rr = search_by_size(size, file_path)
                    if not rr:
                        print('\n***** No file(s) found *****\n')
                    else:
                        break

                except ValueError:
                    print('\n***** Enter an numeric value. *****\n')
                    continue

                except WindowsError:
                    print('\n***** Oops! Access denied. *****\n')
                    continue

The continue statements are redundant here.

Suggested improvements

You can use a strategy pattern to get around having all the if/else constructs. You can define OPERATIONS dict, with your operation identifier as keys and (method_to_be_called, message_to_be_displayed) as values.

OPERATIONS = {
    'n': ('search_by_name', 'Enter the file name'),
    'e': ('search_by_extension', 'Enter the extension'),
    's': ('search_by_size', 'Enter the size'),
    }

if not os.path.exists(file_path):
    print('\n******* Path does not exist. *******\n')
    return  # This way the rest of your code has to be indented less.
while True:
   question = (
       "\nWhich search characteristics would you like to use? "
       "\n 1. Search by name: N "
       "\n 2. Search by extension: E "
       "\n 3. Search by size: S "
       "\n    Please enter your choice:"
       )
   answer = input(question).strip().lower()
   if answer not in OPERATIONS:
       print('\n***** Invalid choice. *****')
       continue
   method, operation_question = OPERATIONS[answer]
   while True:
       value = input(operation_question).strip()
       if answer == 's':  # Search by size.
           try:
               value = int(value)
           except ValueError:
               print('\n***** Enter an numeric value. *****\n')
               continue
       try:
           # This line gets and executes relevant method from the current
           # module (that is what __import__(__name__) stands for).
           result = getattr(__import__(__name__), method)(value, file_path)
       except WindowsError:
           print('\n***** Oops! Access denied.*****\n')
           continue
       if result:
           # Perform actions with a result.
           break
       print('\n***** File not found *****\n')
\$\endgroup\$

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.