7
\$\begingroup\$

I am just wondering if I need to refactor this code into smaller functions, or if it is OK as it is. I know that code should be as clean as possible, but I don't see a need to declare a new function every time I have more than 5 lines of code. Anyways please tell me what you think.

The code itself is used for file handling. It allows a user to create, append, read, or delete files. This is all based on user input.

def option_two():
    #####################################################################
    #   Option 2, File Creation/Editing:                                #
    #   Asks the user what they would like to do                        #
    #   They can view, append, write, or delete a file                  #
    #####################################################################

    path_exists = bool

    try:
        USER_INPUT = int(input("""
        What would you like to do?\n
        1)Read a file
        2)Append to an existing file
        3)Write to a new file
        4)Delete a file
        5)Go back/n
        >>> """))


        if USER_INPUT == 1:
            #View a file
            user_message(1)

            USER_INPUT = input("Is this file located on the desktop? y/n")
            if USER_INPUT == "y":
                USER_INPUT = input("What is the name of the file?")
                path_exists = check_path("//home//alpha//Desktop//{0}".format(USER_INPUT), path_exists)
                if path_exists:
                    open_file("//home//alpha//Desktop//{0}".format(USER_INPUT))
                else:
                    option_two()
            elif USER_INPUT == "n":
                USER_INPUT = input("Enter the full path for the file")
                path_exists = check_path(USER_INPUT, path_exists)
                if path_exists:
                    open_file(USER_INPUT)
                else:
                    option_two()
            else:
                print("Enter y or n only")
                option_two()
            return

        elif USER_INPUT == 2:
            #append to a file
            user_message(2)

            USER_INPUT = input("Is this file located on the desktop? y/n")
            if USER_INPUT == "y":
                USER_INPUT = input("What is the name of the file?")
                path_exists = check_path("//home//alpha//Desktop//{0}".format(USER_INPUT), path_exists)
                if path_exists:
                    append_file("//home//alpha//Desktop//{0}".format(USER_INPUT))
                else:
                    option_two()

            elif USER_INPUT == "n":
                USER_INPUT = input("Enter the full path for the file")
                if path_exists:
                    append_file(USER_INPUT)
                else:
                    option_two()

            else:
                print("Enter a y or n only")
                option_two()
            return

        elif USER_INPUT == 3:
            #Write a new file
            user_message(3)

            USER_INPUT = input("Is this file located on the desktop? y/n")
            if USER_INPUT == "y":
                USER_INPUT = input("What is the name of the file?")
                write_file("//home//alpha//Desktop//{0}".format(USER_INPUT))
            elif USER_INPUT == "n":
                USER_INPUT = input("Enter the full path for the file")
                write_file(USER_INPUT)
            else:
                print("Enter a y or n only")
                option_two()
            return

        elif USER_INPUT == 4:
            #Delete a file
            user_message(4)

            USER_INPUT = input("Is this file located on the desktop? y/n")
            if USER_INPUT == "y":
                USER_INPUT = input("What is the name of the file?")
                delete_path("//home//alpha//Desktop//{0}".format(USER_INPUT), path_exists)
            elif USER_INPUT == "n":
                USER_INPUT = input("Enter the full path for the file")
                delete_path(USER_INPUT, path_exists)
            else:
                print("Enter a y or n only")
                option_two()
            return

        elif USER_INPUT == 5:
            user_message(5)
            print("Moving back to main")
            return

    except(ValueError):
        print("Invalid input, try again\n\n")
        option_two()
\$\endgroup\$
4
  • \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. The current title states your concerns about the code; it needs an edit to simply state the task; see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Sep 13 '18 at 17:48
  • 3
    \$\begingroup\$ @TobySpeight there are an abundance of comments in the code that state exactly what it is doing. My question was clear, should it be refactored into smaller chunks. I am looking for constructive criticism. \$\endgroup\$ – Robotica Sep 13 '18 at 17:55
  • \$\begingroup\$ Sorry, but the comments are not sufficient. There are function definitions missing. So we cannot verify the usage which definitely is inconsistent and buggy. In fact you are asking a doctor to diagnose your BMI only and forcing him to ignore the obvious signs of cancer. Any doctor would refuse to do so. So do we. Your code is really bad. If you want a high quality review that helps you to get better you should provide all code. at least the missing definitions. \$\endgroup\$ – stefan Sep 20 '18 at 1:46
  • \$\begingroup\$ @stefan there's really no need for the bs comments. You don't need function definitions to see whether or not this code needs to be cleaned up (which it did need it very much). If I thought it was great code I wouldn't have bothered to reach out on this site for help? I'm looking for constructive criticism not pointless bs. \$\endgroup\$ – Robotica Sep 20 '18 at 2:22
6
\$\begingroup\$

In my opinion this can be greatly simplified. First, there is no need for this to be a recursive function (which are seldom the best choice in Python due to the maximum recursion depth). Just make it an infinite loop.

Second, all of your options are almost the same. You ask if the file is on the desktop, then for the file name/path and then do something with that path. So, just define a function for that first part:

USERNAME = "alpha"

def ask_file_name():
    user_input = input("Is this file located on the desktop? y/n")
    if user_input.lower() == "y":
        path = "/home/{}/Desktop/{}"
        return path.format(USERNAME, input("What is the name of the file?"))
    return input("Enter the full path for the file")

Then your main function becomes rather short:

MENU = """
What would you like to do?

1)Read a file
2)Append to an existing file
3)Write to a new file
4)Delete a file
5)Go back/n
>>> """

def option_two():
    """Option 2, File Creation/Editing:

    Asks the user what they would like to do
    They can view, append, write, or delete a file
    """

    while True:
        try:
            user_input = int(input(MENU))
        except ValueError:
            print("Invalid input, try again\n\n")
            continue
        user_message(user_input)
        path = ask_file_name()

        if user_input == 1:
            #View a file
            if check_path(path):
                open_file(path)
            else:
                continue
        elif user_input == 2:
            #append to a file
            if check_path(path):
                append_file(path)
            else:
                continue
        elif user_input == 3:
            #Write a new file
            write_file(path)
        elif user_input == 4:
            #Delete a file
            delete_path(path)
        print("Moving back to main")
        return

Note that check_path should work without having a boolean variable passed in and delete_path should probably just call check_path internally (or use try...except).

I also changed your comment under the function definition to conform to Python's docstring convention.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the good info. Idk why I didn't think about putting it in a loop. This isn't exactly related but I've never heard of a max recursion depth? \$\endgroup\$ – Robotica Sep 13 '18 at 19:30
  • \$\begingroup\$ @Robotica: Have a look here: stackoverflow.com/questions/3323001/… By default CPython stops after 1000 levels of recursion (which will be a bit hard to reach in a menu, but is not impossible...) \$\endgroup\$ – Graipher Sep 13 '18 at 19:32
  • 1
    \$\begingroup\$ IMHO it would be nicer to eliminate all the continue's and explicitly return wherever necessary. \$\endgroup\$ – IMil Sep 14 '18 at 2:09
  • 1
    \$\begingroup\$ @IMil Feel free to write your own answer :-) \$\endgroup\$ – Mast Sep 14 '18 at 10:19
  • 1
    \$\begingroup\$ @IMil yes please I would like to see everyone's opinions. \$\endgroup\$ – Robotica Sep 14 '18 at 17:28

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.