2
\$\begingroup\$

You guess heads or tails by clicking one of the buttons on easyGUI. If it's right, you will get a "good job" message. If it's wrong, you will get a "wrong" message! After that, there is an option to play again.

Please give me some feedback on how I can make my code better, if there is room for improvement.

import random
import time
import easygui
import sys
while True:
    rand = random.choice(["Heads", "Tails"])
    firstguess = easygui.buttonbox("Pick one", choices= ["Heads", "Tails"])
    if firstguess == rand:
        easygui.msgbox("Wow you win!")
    else:
    easygui.msgbox("Sorry you guessed wrong!")
    time.sleep(2)
    answer = easygui.buttonbox("Play again?", choices=["Yes","No"])
    if answer == "Yes":
        pass
    else:
        break

easygui.msgbox("Ok, see you later!")
sys.exit(0)
\$\endgroup\$
1
  • 2
    \$\begingroup\$ please, correct your indentation \$\endgroup\$
    – Olayinka
    Sep 10 '13 at 1:06
5
\$\begingroup\$

Your script isn't bad. But there is always room for improvement:

  • It's bad to mix tabs and spaces. The fact that your code broke when you pasted it here shows why. Good editors have a setting that will help you with this. (It is recommended that you use spaces only.)

  • You don't need sys.exit(0) at the end of your script, since Python will exit automatically when it reaches the end.

  • Choose your variable names carefully. rand is not very descriptive and firstguess is plain incorrect (every guess, not just the first, gets assigned to this variable).

  • Don't repeat yourself. Don't write ["Heads", "Tails"] twice, because if you have to change it for whatever reason, you have to remember to change it in two places. Store it in a variable.

  • You can simplify this:

    if answer == "Yes":
        pass
    else:
        break
    

    To this:

    if answer != "Yes":
        break
    
  • I don't know easygui, but it seems you should use a ynbox when you're asking "yes or no?". So you can change this:

    answer = easygui.buttonbox("Play again?", choices=["Yes","No"])
    if answer != "Yes":
        break
    

    To this:

    play_again = easygui.ynbox("Play again?")
    if not play_again:
        break
    

    Or simply:

    if not easygui.ynbox("Play again?"):
        break
    

With all the changes made and after applying the rules from the the Python style guide PEP8, your code looks like this:

import random    # alphabetic order
import time

import easygui    # third-party modules after standard-library modules

CHOICES = ["Heads", "Tails"]    # ALL_CAPS for variables whose value never changes

while True:
    result = random.choice(CHOICES)
    guess = easygui.buttonbox("Pick one", choices=CHOICES)
    if guess == result:
        easygui.msgbox("Wow you win!")
    else:
        easygui.msgbox("Sorry you guessed wrong!")
    time.sleep(2)
    if not easygui.ynbox("Play again?"):
        break

easygui.msgbox("Ok, see you later!")
\$\endgroup\$
1
  • \$\begingroup\$ Thanks so much for the feedback. I have never used "if not" in my code yet so I will try that out. Also I see why I should make the ["Heads, "Tails"] a variable. Thank you! \$\endgroup\$
    – TheSuds13
    Sep 10 '13 at 5:02
1
\$\begingroup\$

Top-level code in a module should generally be definitions only, not "executable" code. In other words, you should be able to import your module without running your program. This makes it easier to read and easier to debug. My preferred style for a script (as opposed to library) module is something like:

"""This docstring explains the program's purpose.

Here's some more details, including descriptions of any command-line arguments.
"""

# Keep imports in sorted order
import bar
import foo
import sys

# Define helper functions, classes, etc
def baz(...):
    """Functions and classes get docstrings too."""
    ...

def main(argv):
    ...

if __name__ == '__main__':
    main(sys.argv)  # This passes commandline arguments to your main function

From a UX point of view, even keeping in mind that this is meant to be a simple program, you could stand to improve. For example, you can always print the last "You guessed correctly" / "You guessed wrong" answer over your dialog box, and make your dialog "Heads"/"Tails"/"Quit", taking away the 2-second delay, and allowing you to quit at any time, not just when the "play again" dialog is up.

\$\endgroup\$
1
  • \$\begingroup\$ I have absolutely no idea how to use the def function correctly. I just don't understand the use for it..would you mind explaining it more? \$\endgroup\$
    – TheSuds13
    Sep 10 '13 at 5:04

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.