Python Forum
Code Review: Range and Enumerate
Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Code Review: Range and Enumerate
#1
Hello

New member here! I just started reading Python Programming: An Introduction to Computer Science by John Zelle. I am about four chapters in and decided to pause and try to write a simple program with the concepts introduced so far.

This isn't really 'homework' but it is very beginner and I am limited in the concepts I know, hence selecting this area to post the question in.

The code below works fine but I don't think the solution is very elegant. Specifically on two points:

* I had a challenge thinking how to prompt the user to enter the 'nth' value. (Lines 35-52). In the end, I ended up creating a list. However, this would not scale well; if I wanted the user to enter forty names, I would need to write 40 lines of code.

* Similarly, I didn't want to create List3 to store the results. It feels strange that I have a list which just holds the results. I perhaps need a new concept..

Anyway, constructive criticism welcome on these couple of points. Regardless, happy to have stretched myself and finished the first small program. Will keep pressing ahead with the textbook :)


from graphics import *

def main():

    #Setup GraphWin
    win = GraphWin("Alastair's LoTR Name Evaluator", 500,500)
    win.setCoords(0,0,10,10)

    #Draw the text
    Text(Point(4.5,9), " This will let you evaluate between 1 and 5 names!").draw(win)
    Text(Point(4.5,8.5), " How exciting!!").draw(win)
    Text(Point(4.5,7), " How many names would you like to evaluate?").draw(win)

    #Draw the input box
    inputText = Entry(Point(4.5,6.5),8)
    inputText.setText("<TYPE>")
    inputText.draw(win)

    #Initialise the output text
    outputText = Text(Point(4.5,5),"")
    outputText.setText("<Waiting for input...>")
    outputText.draw(win)
    
    #Create two empty lists
    List1 = []
    List2 = []
    List3 = []

    #Wait until a key is pressed
    win.getKey()

    #Set the variable 'quantity_of_names' to be the number in the inputText element
    quantity_of_names = inputText.getText()
    
    #Adjust the list (List2) based on the number of names entered by the user 
    if quantity_of_names == "1":
        List2 = ["first"]
       
    elif quantity_of_names == "2":
        List2 = ["first","second"]
        
    elif quantity_of_names == "3":
        List2 = ["first","second","third"]
       
    elif quantity_of_names == "4":
        List2 = ["first","second","third","fourth"]
       
    elif quantity_of_names == "5":
        List2 = ["first","second","third","fourth","fifth"]
       
    else:
        unacceptable = "Warning: You didn't provide an acceptable input!!"

        #Set the output text to be 
        outputText.setText(unacceptable)

    #Draw the second input box
    inputTextName = Entry(Point(4.5,4),20)
    inputTextName.setText("")
    inputTextName.draw(win)
       
    # Loop through list (List2) to ask the user to enter the names
    # Append each name entered by the user into list (List1)
    for elements, count in enumerate(List2):
        string = ("Enter the " + List2[elements] + " name: ")
        outputText.setText(string)

        #User expected to type into box here
        #Click Mouse to advance
        win.getMouse()

        #Set variable 'name' to be the input 
        name = inputTextName.getText()

        #Add the name to the list
        List1.append(name)

        #Reset the text boxes during the loop
        inputTextName.setText("")
        outputText.setText("Thanks for adding your names. Results below")

    #Removes the input box for user to add any more names
    inputTextName.undraw()

    #Loops through the list (List1) 
    for name in list(List1):
        if name == "Aragorn":
            result = "Ranger of the north"

        elif name == "Frodo":
            result = "Hobbit"

        elif name == "Gandolf":
            result = "Wizard"

        elif name == "Legolas":
            result = "Elf"

        else:
            result = "Race unknown..."

        List3.append(result)

    #Determine the length of the list of names
    length = len(List1)
    print(length)

    #Create new text boxes to display the names and results!
    Rectangle(Point(1.0,3.5),Point(9.0,0.5)).draw(win)
    firstNameBox = Text(Point(4.5,3),("NAME: " + List1[0] + "  //  RESULT: " + List3[0])).draw(win)
    secondNameBox = Text(Point(4.5,2.5),("NAME: " + List1[1] + "  //  RESULT: " + List3[1])).draw(win)
    thirdNameBox = Text(Point(4.5,2),("NAME: " + List1[2] + "  //  RESULT: " + List3[2])).draw(win)
    fourthNameBox = Text(Point(4.5,1.5),("NAME: " + List1[3] + "  //  RESULT: " + List3[3])).draw(win)
    fifthNameBox = Text(Point(4.5,1),("NAME: " + List1[4] + "  //  RESULT: " + List3[4])).draw(win)
Reply
#2
Constructive criticism / suggestions follows (not familiar with graphics, so no comments about that):

(1) Don't use star-import (from graphics import *).

Why? From python.org:

Quote:Note that in general the practice of importing * from a module or package is frowned upon, since it often causes poorly readable code. However, it is okay to use it to save typing in interactive sessions.

(2) If updating code update comments as well

You have on row #24 comment: '#Create two empty lists' and below that you create three empty lists. This makes reader of the code wonder - which is wrong: code or comment

(3) Use descriptive names.

Why? List1, List2, List3 give no hint what data is kept here and/or on what purpose. It makes understanding of code harder.

(4) Instead of chained if-s use suitable datastructure.

Why? Chained if-s are hard to debug and not scalable. Usually dictionary or list are good alternatives.

>>> names = ['first', 'second', 'third', 'fourth']
>>> qty_of_names = 3       # in your code you can convert str -> int
>>> names[:qty_of_names]   # slice
['first', 'second', 'third']
More names can be appended to the list of names without need to change anything else.

(5) Why you use main()?

Usually it's used for top-down style but I didn't observe it here.

(6) Have respect to the Fellowship of the Ring! Wink

Why? There is no Aragon or Gandolf! Besides that one can use dictionary instead of chained if-s. More characters can be added without need to change the other parts of the code:

>>> characters = {'Aragorn': 'Ranger of the north', 'Frodo': 'Hobbit', 'Gandalf': 'Wizard', 'Legolas': 'Elf'}
>>> name = 'Frodo'
>>> try:
...     result = characters[name]
... except KeyError:
...     result = 'Race unknown...'
... 
>>> result
'Hobbit'
>>> name = 'Pippin'
>>> try:
...     result = characters[name]
... except KeyError:
...     result = 'Race unknown...'
... 
>>> result
'Race unknown...'
One should wrap this into function.
I'm not 'in'-sane. Indeed, I am so far 'out' of sane that you appear a tiny blip on the distant coast of sanity. Bucky Katt, Get Fuzzy

Da Bishop: There's a dead bishop on the landing. I don't know who keeps bringing them in here. ....but society is to blame.
Reply
#3
Thank you very much for this. All useful and helpful points - especially #1 and the points on dictionaries!
Reply


Possibly Related Threads…
Thread Author Replies Views Last Post
  Python Code Review and Challenge cpatte7372 2 1,946 Jan-14-2022, 10:45 AM
Last Post: cpatte7372
  Please review and help me on this code Sushmakusu 4 2,147 Aug-05-2021, 11:37 AM
Last Post: jamesaarr
  Alternative of this mini code without Enumerate and join function. Ace4Benji 2 2,436 Mar-09-2020, 08:22 PM
Last Post: micseydel
  Requesting homework help and maybe a little code review… JonnyEnglish 0 1,558 Jan-16-2020, 04:34 PM
Last Post: JonnyEnglish
  How to transform an enumerate object into a dictionary fad3r 7 4,573 Feb-11-2018, 11:42 PM
Last Post: Larz60+
  enumerate and output control atux_null 7 5,040 Oct-24-2017, 06:50 PM
Last Post: Mekire

Forum Jump:

User Panel Messages

Announcements
Announcement #1 8/1/2020
Announcement #2 8/2/2020
Announcement #3 8/6/2020