8
\$\begingroup\$

As part of my autodidacticism into all things Python related, I have decided to write a number of functions that I can save in a library for future use. One such function that struck me as useful to have, would be a function that could be called each time the user wished to make an amendment to an existing file.

The result.

def amend_file_data(file_name) : # file_name to incl. extension. 
    #Opens existing file and amends the data according to user specifications.
    #Local declarations: 
    file_contents = []
    line_number = ""
    amendments_complete = False
    finalised_content = ""
    further_amendments = ""
    new_file_name = ""
    check_intention = ""

    file_contents = display_file_contents(file_name) # Displays contents of file and returns the contents as a list. 
    while not amendments_complete :
        line_number = input("Please enter line number for amendment. \n"\
            "To exit, please enter 'x': ")
        while line_number.lower() != "x" :
            line_number = int(line_number)
            file_contents[line_number] = input("\nPlease enter replacement data: ") + "\n"
            line_number = input("\nPlease enter line number for amendment. \n"\
                "To exit, please enter 'x': ")
        finalised_content = "".join(file_contents)
        print("\nFile contents are now:\n" + finalised_content)
        further_amendments = input("Do you wish to make any further amendments (Y/N): ")
        if further_amendments.lower() == "n" :
            amendments_complete = True
    new_file_name = input("\nWould you like to keep the same file name? Y/N: ")
    if new_file_name.lower() == "y" :
        check_intention = input("\nPlease note, all previous data will be overwritten,"\
            " do you want to proceed? Y/N: ")
        if check_intention.lower() == "y".lower() :
            with open(file_name,"w+") as new_file :
                new_file.write(finalised_content)
        else: 
            create_new_file(finalised_content)
    else:
        create_new_file(finalised_content)
    return print("\nWriting to file complete. ")

def create_new_file(finalised_content) : #Function accepts argument as string. 
    #Creates a new file, accepting the file contents as an argument. 
    #Local declarations:
    new_file_name = ""

    new_file_name = input("\nPlease enter a new file name (incl. .ext): ") #File name to include .ext
    with open(new_file_name,"w+") as new_file :
        new_file.write(finalised_content)
    return print("New file created successfully.")

def display_file_contents(file_name) : # file_name to include .ext
    #Displays contents of file to user and returns contents of file as a list.  
    #Local declarations:
    file_contents = []
    display_contents = ""

    with open(file_name,"r+") as file_for_display :
        file_contents = file_for_display.readlines()
        display_contents = "".join(file_contents)
        print("Current contents of file:\n" + display_contents)
    return file_contents #Returning file contents adds flexibility to the function, and allows
                     #further interaction with other functions. 

The function(s) work(s) as intended (I acknowledge there are limitations as to its use).

My Problem:

Because I am teaching myself programming (initially Python), it is very difficult for me to know if my code is ‘good’ code. Yes, it works; however, from what I have garnered there are numerous other considerations to take into account, such as readability, modularity and efficiency.

My Question:

My function, amend_file_data() bothers me. I understand functions should be succinct.

  1. Is my function too long? Is this question answerable, or is it subjective in nature? What would be the general consensus among more experienced programmers?

  2. If the answer is “yes, it is too long”, then what is the solution? The function does everything I want it to, no more, no less.

  3. Could the function be structured better, could it look better, read better. If yes, how please?

  4. When does a function become a program? This is perhaps the overriding question for me. On the one hand I think my function is fine; it could be called in numerous different circumstances, and incorporated into other programs. On the other hand, it appears as a self-contained program; a program to run which enables the user to update files.

    Should functions be limited? In my case, my function does perform several different actions, however they are all part of what I want the function to do; display the contents of the file to the user and allow him/her to execute amendments. I am highly confused, am I over thinking it!

  5. Should I add error handling to my function? If so, it really will start to become very long indeed.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ "When does a function become a program?" Python being a scripting language, there isn't much difference between the two. \$\endgroup\$
    – Mast
    Feb 5, 2016 at 11:58
  • 1
    \$\begingroup\$ What functional concepts did you use in your code? To me it looks like your code is imperative. \$\endgroup\$
    – Peilonrayz
    Feb 5, 2016 at 12:51
  • \$\begingroup\$ @JoeWallis Thank you for your comment. You'll have to excuse my ignorance, I have absolutely no formal computer science education, and as of yet I am not familiar with the either term functional concepts or imperative, in this context. I've just done a quick search however, and it looks like I'll need to do some reading on the differences between Imperative and Declarative, so thank you for bringing it to my attention. \$\endgroup\$ Feb 5, 2016 at 13:00
  • 3
    \$\begingroup\$ @mugman No problem, you can do functional programming in Python, which is why I checked. I've removed the tag, but have fun learning! \$\endgroup\$
    – Peilonrayz
    Feb 5, 2016 at 13:11

2 Answers 2

7
\$\begingroup\$

Docstring

The purpose and arguments for your functions should be documented using docstring.

Local declaration

You have habits of languages requiring variable declarations but there is not such thing in Python (even in languages requiring it, I consider it is better to declare variables as late as possible, in the smallest possible scope).

Style

Python has a style guide called PEP 8. You have various points that need to be changed to be compliant, mostly related to spacing. On the other hand, your are properly following the naming convention.

First draft

Once the comments above taken into account, the code looks like :

def amend_file_data(file_name):
    """Opens existing file and amends the data according to user specifications.
    file_name to incl. extension."""
    finalised_content = ""
    file_contents = display_file_contents(file_name)  # Displays contents of file and returns the contents as a list.
    while True:
        line_number = input("Please enter line number for amendment. \n"
            "To exit, please enter 'x': ")
        while line_number.lower() != "x":
            line_number = int(line_number)
            file_contents[line_number] = input("\nPlease enter replacement data: ") + "\n"
            line_number = input("\nPlease enter line number for amendment. \n"
                "To exit, please enter 'x': ")
        finalised_content = "".join(file_contents)
        print("\nFile contents are now:\n" + finalised_content)
        further_amendments = input("Do you wish to make any further amendments (Y/N): ")
        if further_amendments.lower() == "n":
            break
    new_file_name = input("\nWould you like to keep the same file name? Y/N: ")
    if new_file_name.lower() == "y":
        check_intention = input("\nPlease note, all previous data will be overwritten,"
            " do you want to proceed? Y/N: ")
        if check_intention.lower() == "y".lower():
            with open(file_name, "w+") as new_file:
                new_file.write(finalised_content)
        else:
            create_new_file(finalised_content)
    else:
        create_new_file(finalised_content)
    return print("\nWriting to file complete. ")


def create_new_file(finalised_content):
    """Creates a new file, accepting the file contents as an argument.
    #Function accepts argument as string."""
    new_file_name = input("\nPlease enter a new file name (incl. .ext): ")  # File name to include .ext
    with open(new_file_name, "w+") as new_file:
        new_file.write(finalised_content)
    return print("New file created successfully.")


def display_file_contents(file_name):
    """Displays contents of file to user and returns contents of file as a list.
    file_name to include .ext"""
    file_contents = []
    with open(file_name, "r+") as file_for_display:
        file_contents = file_for_display.readlines()
        display_contents = "".join(file_contents)
        print("Current contents of file:\n" + display_contents)
    return file_contents  # Returning file contents adds flexibility to the function, and allows
    # further interaction with other functions.

Duplicated logic

The following code does not respect the Don't Repeat Yourself principle:

    line_number = input("Please enter line number for amendment. \n"
        "To exit, please enter 'x': ")
    while line_number.lower() != "x":
        line_number = int(line_number)
        file_contents[line_number] = input("\nPlease enter replacement data: ") + "\n"
        line_number = input("\nPlease enter line number for amendment. \n"
            "To exit, please enter 'x': ")

I think it would be more clear to have :

    while True:
        line_number = input("Please enter line number for amendment. \n"
            "To exit, please enter 'x': ")
        if line_number.lower() == "x":
            break
        line_number = int(line_number)
        file_contents[line_number] = input("\nPlease enter replacement data: ") 

User experience

From a user point of view, it would probably make sense to stop amendments when x ig given as a line_number. Having 2 questions asked to exit is a bit un-natural.

Separation of concerns

It is a bit weird to have the create_new_file function to ask the user for a file name. Also, we have the open("w+") and write logic in 2 places. A better idea would be to have the file name given to the create_new_file function.

Performing simple substitutions, we have :

    if new_file_name.lower() == "y":
        check_intention = input("\nPlease note, all previous data will be overwritten,"
            " do you want to proceed? Y/N: ")
        if check_intention.lower() == "y".lower():
            create_new_file(file_name, finalised_content)
        else:
            file_name = input("\nPlease enter a new file name (incl. .ext): ")
            create_new_file(file_name, finalised_content)
    else:
        file_name = input("\nPlease enter a new file name (incl. .ext): ")
        create_new_file(file_name, finalised_content)
    return print("\nWriting to file complete. ")


def create_new_file(file_name, content):
    """Creates a new file, accepting the file contents as an argument.
    #Function accepts argument as string."""
    with open(file_name, "w+") as new_file:
        new_file.write(content)
    return print("New file created successfully.")

But it would be a good idea to remove duplicated logic.

keep_file_name = False
new_file_input = input("\nWould you like to keep the same file name? Y/N: ")
if new_file_input.lower() == "y":
    check_intention = input("\nPlease note, all previous data will be overwritten,"
        " do you want to proceed? Y/N: ")
    keep_file_name = check_intention.lower() == "y".lower()
new_file_name = file_name if keep_file_name else input("\nPlease enter a new file name (incl. .ext): ")
create_new_file(file_name, finalised_content)

Separation of concerns (again)

Your display_file_contents does 2 things : getting content and displaying it. It is a bad idea and you felt it and you needed to document it as you were calling the function. It is probably clearer to just get the content and then display it from somewhere else. Then the function name would obviously need to be changed.

Updated code

def amend_file_data(file_name):
    """Opens existing file and amends the data according to user specifications.
    file_name to incl. extension."""
    file_contents = get_file_content(file_name)  # Displays contents of file and returns the contents as a list.
    finalised_content = "".join(file_contents)
    print("Current contents of file:\n" + display_contents)
    while True:
        line_number = input("Please enter line number for amendment. \n"
            "To exit, please enter 'x': ")
        if line_number.lower() == "x":
            break
        line_number = int(line_number)
        file_contents[line_number] = input("\nPlease enter replacement data: ")
        finalised_content = "".join(file_contents)
        print("\nFile contents are now:\n" + finalised_content)
    keep_file_name = False
    new_file_input = input("\nWould you like to keep the same file name? Y/N: ")
    if new_file_input.lower() == "y":
        check_intention = input("\nPlease note, all previous data will be overwritten,"
            " do you want to proceed? Y/N: ")
        keep_file_name = check_intention.lower() == "y".lower()
    new_file_name = file_name if keep_file_name else input("\nPlease enter a new file name (incl. .ext): ")
    create_new_file(file_name, finalised_content)


def create_new_file(file_name, content):
    """Creates a new file, accepting the file contents as an argument.
    #Function accepts argument as string."""
    with open(file_name, "w+") as new_file:
        new_file.write(content)
    return print("New file created successfully.")


def get_file_content(file_name):
    """Returns contents of file as a list.
    file_name to include .ext"""
    file_contents = []
    with open(file_name, "r+") as file_for_display:
        file_contents = file_for_display.readlines()
    return file_contents
\$\endgroup\$
5
  • \$\begingroup\$ Definitely agree with you on the separation of concerns front. What do you think about create_new_file returning an exit status such as True on successful file creation and False otherwise? \$\endgroup\$
    – Ogaday
    Feb 5, 2016 at 15:05
  • \$\begingroup\$ @Josay Thank you. Please could you clarify one point. You mention that my function display_file_contents does two things, and that I should separate concerns; which I agree with. However, by moving the 'display' part of the function into the amend_file_data function, it is this function that now becomes multifaceted and does not properly separate concerns! It is this that I am struggling with. Why separate out concerns in one function but not another? I suppose it sits more naturally in the amend_file_data function, as it is purposefully used in this context. \$\endgroup\$ Feb 6, 2016 at 18:05
  • \$\begingroup\$ @mugman Indeed, one could easily say that amend_file_data technically does more than one thing. However, it does not do much more than what its name suggests and my point was that the finalised_content = "".join(file_contents) then print(finalised_content) piece of logic was already performed in that same function. \$\endgroup\$
    – SylvainD
    Feb 7, 2016 at 17:15
  • \$\begingroup\$ @Josay Thank you for all your feedback, it's very valuable to me as I am learning on my own. \$\endgroup\$ Feb 8, 2016 at 19:03
  • \$\begingroup\$ I'm glad it is helping :-) \$\endgroup\$
    – SylvainD
    Feb 8, 2016 at 19:04
4
\$\begingroup\$

This is my first time posting on this site so I hope this is ok:

I would just like to comment on a couple of style points:

  1. Store your strings at the head of the file. This will make your code more legible and easier to maintain. for instance, "To exit, please enter 'x': " is repeated in multiple places in your code. If you define exit_txt = "To exit, please enter 'x': " at the top of your file, and then use input(exit_txt) your code will become more legible and if you want to change that string in the future, you will only need to change it in one place. I would recommend doing this with all of your strings in this case, even if you only use them once. This adheres to the principle of DRY - don't repeat yourself.

  2. Josay has good points about using docstrings and not declaring variables before you use them: ie. There is no need to define line_number = "" before line_number=input(...). You do need to keep amendments_complete = False for instance, however.

  3. return print(...) is a strange structure. print is a function that outputs the string to stdout and returns None. return returns the vale of the expression following it as the value of the function. If there is no return statement, when the function exits it returns None by default, so the return on the last line of amend file data has no purpose. In other cases, in the middle of your code, for instance, when you want the the function to terminate, you can use return, and an return followed by nothing exits the function and the function also evaluates to None. Stylistically, I would suggest removing some returns where you don't need them, and in other cases use print(...) (newline) returnso you (or others) don't get confused and think you are trying to return the value of the string being printed.

  4. When does a function become a program? Currently, your program is a collection of functions. When you type python mugmans_file_amending_program.py into the command line, the program will define the functions and exit. To make it a bit more interesting, but also allow you import the functions into other code without doing anything else, you can use the __name__ property. Essentially, __name__ is set to the value "__main__" when it is run directly, and to the name of the module otherwise. That means you can use the following structure:

    if __name__=="__main__":
        # code to be run when the file is run directly but not imported.
    

so that your code prompts you when you run it, for instance. There are a few examples here.

  1. Even though you don't like the idea, depending on how much you're planning to use this, I would recommend handling errors sanely. Even you will make mistakes when using the software, maybe mistyping etc. and especially when handling file io want to make sure that your process doesn't fail halfway through editing a file.

  2. Your code is probably fine as is, however, if you feel like refactoring, try to think about separation of "business logic" and "programming logic". For instance, I would suggest create_new_file should accept filename as an argument, therefore it would do exactly what it says on the tin. So other functions that call it, such as amend_file_data or a hypothetical main function would query the user for a file name, gather the file body somehow and then pass it to create_new_file. I would argue that this makes your code easier to understand and maintain, again.

In summation, I think my last point is the most important. Ideally you would have one function which does the "business logic" - the list of conditionals that checks whether you want to amend or create a file etc. which then calls the smaller units of functionality to act on the files themselves.

I hope all that makes sense! If not, please ask me questions and I'll do my best to answer them.

edit: it appears Josay has edited his answer to include the seperation and don't repeat yourself principles just before I posted mine, I'm going to take that as a good sign that I'm on the right track myself. I agree with everything that he has posted as well.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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