10
\$\begingroup\$

I'm teaching myself programming, using Python as my initial weapon of choice.

I have learnt a few basics and decided to set myself the challenge of asking the user for a list of names, adding the names to a list and then finally writing the names to a .csv file.

What would you do differently, i.e. how could this code be improved for readability and efficiency? Would you approach the situation differently, structure it differently, call different functions? I am interested in, and would appreciate a great deal, the feedback from more experienced programmers.

In particular, I find certain parts clunky; such as having to specify to the user the required format for data entry. If I were to simply request the data (name age location) without the commas however, then each record, when written to .csv, would simply end up as one record per cell (Excel) – this is not the desired result.

guestNames = ""
guestList = []
number = 0
guestNumber = ["second","third","fourth","fifth","sixth"]
finalList = ""

#Requesting user input. 
guestNames = input("Please enter the names of your guests, one at a time.\n"\
    "Once you have finished entering the information, please type the word \"Done\".\n"\
    "Please enter your names in the following format (Name, Age, Location). ").capitalize()

guestList.append(guestNames)

while guestNames.lower() != "done".lower() :
    guestNames = input("Please enter the name of your " + guestNumber[number] + " guest: ").capitalize()
    guestList.append(guestNames)
    number += 1

#Sorting the list. 
guestList.sort()
guestList.remove("Done")

#Creating .csv file. 
guestFile = open("guestList.csv","w")
guestFile.close()

#Writing to file. 
for entries in guestList :
    guestFile = open("guestList.csv","a")
    guestFile.write(entries)
    guestFile.write("\n")
    guestFile.close()
\$\endgroup\$
6
\$\begingroup\$

Here is just a quick response to your code:

  • Look into the csv module – This module handles csv-files rather nicely
  • Look into using functions to separate concerns – At all the places you've added comments, you should/could use functions. And the comments should rather be docstrings on line after function definition like in the following:

    def write_list_to_file(guest_list, filename):
        """Write the list to csv file."""
    
        with open(filename, "w") as outfile:
            for entries in guest_list:
                outfile.write(entries)
                outfile.write("\n")
    
  • Variable and function names are usually snake_case – See example above, which is according to PEP8

  • Use the with construct to encapsulate file handling – This is called a context manager, and it handles error cases, opening and closing of the file. See example above.
  • Why do you create the file, and not write to it directly? – I don't quite see why you do the # Creating .csv file, when you can both create and write to it directly.
  • Change text to avoid guestNumber – Having the guestNumber seems unneccessary. If you change the text, you don't need it. Using something similar to "Add another guest (Name, age, location) or 'Done': ".
  • Doing capitalize() is somewhat "dangerous" – What if "John McEnroe" comes along, or "Jean von Luxembourg"? They wouldn't like their name changed into "John Mcenroe" or "Jean Von Luxembourg". The same applies to the location part.
  • In general, don't add stuff you're going to remove immediately afterwards – That is I would most likely not add the Done guest for later removal, instead of adding it first and then removing it. Add a simple if in front of the append and the issue is avoided.
  • Avoid lengthy input prompts – Maybe personal preference, but I would rather have a lengthy text in front of the input prompt, instead of a lengthy prompt text. This way you could possibly also give even more examples of legal input.
  • Change into separate inputs – If you change to using the csv module, I would also change to three separate input statements, and possible do some simple validation on each of the inputs before adding them into the guest list.
  • Careful think about variable names – Some of your names are good, and some not so good. I.e. guestNames when it actually is both the name, age and location, is misleading. And in for entires in guestList, it would possibly read easier as for guest in guest_list:.

    The latter is a typical pattern by the way, that is to use the singular of something plural, i.e. for guest in guests:, for number in numbers:, and so on. In general, plural version indicates list of some sort, whilst the singular version can be used within loops and similar.

I'll end this hasty review with a very useful construct which could cleanup your code (when you've created functions):

def main():
    """Main method used when called as a script."""

    guests = enter_guests()
    write_list_to_file(sorted(guests), "guestlist.csv")


if __name__ == '__main__':
    main()

Using this construct would allow for your code to be reused as a module later on, whilst still allowing it to be run directly from the command line. It also makes it very easy to understand what happens just skimming the code.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you for this. I find it extremely valuable to receive feedback of this kind, as it teaches me to think more about the structure of the code and best practices, as opposed to just writing a piece of code that ‘works’. Thanks again. \$\endgroup\$ – Andrew Hardiman Jan 6 '16 at 19:52
  • \$\begingroup\$ @mugman do feel free to work on a second version, and then post that version as a follow up some day. \$\endgroup\$ – holroy Jan 6 '16 at 21:32

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.