Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

In the past few days i have started to learn python 3 in my spare time and i am really enjoying it so far.

I have attempted many simple programs such as a palindrome checker and reverse a string in an attempt to improve my skills.

I was hoping that someone would be able to provide some feedback on the following program which is a guessing game program. The user is able to choose the number of guesses and the max possible value.

From my own perspective i think that possibly i am relying too much on while loops and if/else statements. Also i am not sure if my comments are sufficient, do they lack detail? Also i use parentheses around the conditionals in my if/else statements and in my while loops as i find it easier to read the condition this way. Is this a bad idea? Could it confuse people who are trying to read my code?

# In this program, the user must try to guess a random number generated by the computer
from random import randint

def guess_game():
    replay = 1   
    #Replay allows a user to replay the game without having to call the function again. 
    #The values are: 1 = Replay, any other character = End program
    while (replay == 1):       
        #lives = Number of chances you have to correctly guess the answer before the program finishes
        lives = int( input("How many lives would you like? ") )
        #max_value = upper bound for the randomly generated number. Starts at 0
        max_value = int( input("Please choose the largest number that can be generated: ") )
        random = randint(0, max_value)  #Generates a random int between 0 and max_value
        num_of_guesses = 1  #keeps track of the number of guesses made by the user
        while (num_of_guesses <= lives):
            guess = int( input("Please enter your guess: ") )
            if (guess == random):
                print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
                break  #if answer is correct this will exit the while loop
            else:
                print("Unlucky, that guess was incorrect. You have", (lives-num_of_guesses), "lives remaining.\n")
            num_of_guesses = num_of_guesses + 1
        if (num_of_guesses > lives):
            print("You have run out of guesses. The correct answer was:", random)
        replay = int( input("\nWould you like to play again? Enter 1 for yes, any other character for no: ") )
        if (replay != 1):
            print("Thank you for playing. See you next time")

guess_game()
share|improve this question
    
you are incrementing an int in a loop until it reaches a given number. you can simplify that by using a for loop. in your case, for num_of_guesses in range(1, lives + 1): – njzk2 Jan 15 at 17:11
    
avoid naming variables like keyword or packages. in your case, avoid naming a variable random. instead, something descriptive of its purpose, maybe value_to_guess. – njzk2 Jan 15 at 17:13
    
you could extract the whole replay part in another function. it is not strictly part of the game. – njzk2 Jan 15 at 17:14
up vote 5 down vote accepted

Functions

Everything is in one function. You should stick to the single responsibility principle. Basically, a function should have one job.

In a way, that does mean your whole game should be in the function, but it'd be better to have parts of it separated out. For example, getting an int from the user could be a function:

def get_int(message)
    return int(input(message))


lives = get_int("How many lives would you like? ")
max_value = get_int("Please choose the largest number that can be generated: ")

Now this may seem unnecessary, but now you could add input validation. Caridorc pointed out a bug, so you should do error handling. The best way to do this in Python is to catch any ValueErrors that get thrown.

    try:
        return int(input(message))
    except ValueError:
        print("Please enter an integer only")

Now the code wont crash when an invalid string is entered, but it also wont return a value. So you want to loop over this with while until a valid number is entered and returned, ending your loop. This is how it would look:

def get_int(message)
    while True:
        try:
            return int(input(message))
        except ValueError:
            print("Please enter an integer only")

Now you have error handling set up, but you can still use one line to get each value:

lives = get_int("How many lives would you like? ")
max_value = get_int("Please choose the largest number that can be generated: ")

Pythonic style

While we're on while, there's a more Pythonic way to do your main game loop. Instead of using replay, just use another infinite loop with while True and end your loop with a break, like this:

if replay != 1:
    print("Thank you for playing. See you next time")
    break

Also you don't need brackets around the condition, Python looks nicer without them and evaluates it just fine.

Instead of using

num_of_guesses = num_of_guesses + 1

use Python's += operator:

num_of_guesses += 1

In fact, instead of that at all, you could just use a for loop instead of a while loop here:

for num_of_guesses in range(1, lives + 1):
    guess = get_int("Please enter your guess: ")
    if guess == random:
        print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
        break  #if answer is correct this will exit the while loop
    else:
        print("Unlucky, that guess was incorrect. You have", (lives - num_of_guesses), "lives remaining.\n")
if num_of_guesses > lives:
    print("You have run out of guesses. The correct answer was:", random)

range lets you just loop over a list of numbers. In this case, it loops from one up to lives. Range will iterate up to but not include the second parameter, which means if you pass lives + 1 the last loop will be on the value of lives.

You could also make use of Python's else clause for for loops. An else after a for loop in Python will be executed if the loop fully runs it's course without reaching a break command. If break gets called then the else block is skipped. This works perfect for your case, so I'd write it like this:

for num_of_guesses in range(1, lives + 1):
    guess = get_int("Please enter your guess: ")
    if guess == random:
        print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
        break  #if answer is correct this will exit the while loop
    else:
        print("Unlucky, that guess was incorrect. You have", (lives - num_of_guesses), "lives remaining.\n")
else:
    print("You have run out of guesses. The correct answer was:", random)

Readability

You have good names here. They're clear and readable, without being too long or overlapping. Although, I would advise against using random as a variable name. It doesn't cause a clash here since you used from random import randint, but it causes mild confusion. Something like answer would actually be more useful for indicating what the number is for.

However you have a lot of comments that don't need to be here. For example:

        break  #if answer is correct this will exit the while loop

This is quite a verbose comment. At most I would have # Correct answer. People reading this code probably know how break works. Your comments should focus on intent and abstract meaning. Python programmers can read your code and know what each part does, but why it does it, and what you want to do that for are useful to know in comment form. (as a beginner, perhaps you want to remind yourself of what these things do. This may be of benefit to start with, but I suggest reducing them as soon as you feel comfortable with it).

share|improve this answer

Modularization

You may want to let the user play just one guessing game from another program (maybe a module that makes you chose and play your favourite game), the code also becomes simpler if you write a guess_game function that just plays the guessing game, like:

def guess_game():
    lives = int( input("How many lives would you like? ") )

    max_value = int( input("Please choose the largest number that can be generated: ") )
    random = randint(0, max_value)  
    num_of_guesses = 1  
    while (num_of_guesses <= lives):
        guess = int( input("Please enter your guess: ") )
        if (guess == random):
            print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
            break
        else:
            print("Unlucky, that guess was incorrect. You have", (lives-num_of_guesses), "lives remaining.\n")
        num_of_guesses = num_of_guesses + 1
    if (num_of_guesses > lives):
        print("You have run out of guesses. The correct answer was:", random)

You may then call this function in a loop to allow the user to play again, being careful about fixing:

A bug

When you ask:

Would you like to play again? Enter 1 for yes, any other character for no:

The user thinks that entering a non-number is ok, but if I enter for example n:

Would you like to play again? Enter 1 for yes, any other character for no: n
Traceback (most recent call last):
  File "/home/riccardo/guess_name.py", line 29, in <module>
    guess_game()
  File "/home/riccardo/guess_name.py", line 25, in guess_game
    replay = int( input("\nWould you like to play again? Enter 1 for yes, any other character for no: ") )
ValueError: invalid literal for int() with base 10: 'n'

As entering a non-number is valid, it should be handled without crashing.

range

Your while loop iterates a fixed number of times (when the break is not reached, at least), so I suggest using range, in general learning about Python built-ins will greatly simplify your code.

share|improve this answer
    
thank your for your reply. i had not spotted the bug when entering a non-number, i will alter this to: Would you like to play again? Enter 1 for yes, any other integer for no: – php123 Jan 15 at 14:58
    
@php123 Your solution is valid, but it looks more straightforward to keep the input as a string – Caridorc Jan 15 at 15:00

User input

At the moment, if the user inputs an invalid number, a non-handled exception is thrown and your program stops. It could be a good idea to try to handle this. Because this can make your code a bit more complicated, it is probably an even better idea to write a function to handle this job. Something like :

def get_int_from_user(prompt):
    while True:
        try:
            return int(input(prompt))
        except ValueError:
            print("Invalid value")

Duplicated logic

In 2 different places (3 if you count the text in the prompt), you take into account the fact that replay == 1 means "play again". It might be just as easy to have this used in a single place.

For instance,

while True:
    ...
    replay = get_int_from_user("\nWould you like to play again? Enter 1 for yes, any other character for no: ")
    if (replay != 1):
        print("Thank you for playing. See you next time")
        break

Style

You don't need that many parenthesis in Python.

You have way too many comments explaining the obvious. If one reads youre code, he/she is expected to know what break does.

Loops

You are using a while loop to perform iterations but this can just as easily be done with for and range. Also, a quite unknown feature of Python is the fact that for accept an else block which is to be understood as a nobreak.

    for num_of_guesses in range(1, lives+1):
        guess = get_int_from_user("Please enter your guess: ")
        if guess == random:
            print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
            break  #if answer is correct this will exit the while loop
        else:
            print("Unlucky, that guess was incorrect. You have", (lives-num_of_guesses), "lives remaining.\n")
    else: # nobreak
        print("You have run out of guesses. The correct answer was:", random)

Organisation

It could be a good idea to split your program into small functions. Such a function could be used to handle a single game and be called again and again on demand from a function traditionnally named main.

Then the call to the main function (or any code actually doing something (by opposition to just defining)) is usually behind an if name == "main": guard.

Final code

Here the final version of the code:

# In this program, the user must try to guess a random number generated by the computer
from random import randint

def get_int_from_user(prompt):
    while True:
        try:
            return int(input(prompt))
        except ValueError:
            print("Invalid value")

def play_game():
    lives = get_int_from_user("How many lives would you like? ")
    max_value = get_int_from_user("Please choose the largest number that can be generated: ")
    random = randint(0, max_value)
    for num_of_guesses in range(1, lives+1):
        guess = get_int_from_user("Please enter your guess: ")
        if guess == random:
            print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
            break
        else:
            print("Unlucky, that guess was incorrect. You have", (lives-num_of_guesses), "lives remaining.\n")
    else: # nobreak
        print("You have run out of guesses. The correct answer was:", random)


def main():
    while True:
        play_game()
        replay = get_int_from_user("\nWould you like to play again? Enter 1 for yes, any other character for no: ")
        if replay != 1:
            print("Thank you for playing. See you next time")
            break

if __name__ == "__main__":
    main()

Further improvement

It could be a good idea to add proper documentation like docstrings to your module and functions.

share|improve this answer

Welcome to Python :)

Readability

Firstly, Python doesn't require parentheses ((..)) around conditionals. So you can just say:

while replay == 1:

Secondly, too many comments make the code look messy and the reader learns to ignore them. Avoid commenting obvious code, for example:

random = randint(0, max_value)  #Generates a random int between 0 and max_value

The comment is unnecessary. The code is self-explanatory enough, and if the reader doesn't know what randint does, it's time he/she learns it by themselves anyway. Assume the reader knows the language.

Thirdly, logical separation of code into paragraphs greatly helps readability. A 'paragraph' should make a single 'point', and then you should have a blank line before the next paragraph.

Handling unwelcome scenarios

You should handle corner cases in your code.

"Any fool can write code that a computer can understand. Good programmers write code that humans can understand." Martin Fowler

For example, a possible corner case in your program is invalid user input. What happens when the user types 'bla' as a guessed number? A ValueError is raised and crashes your program. You can handle these situations using one of two approaches.

Preferable approach:

try:
    number = int(input())
except ValueError:
    print('Enter a valid integer.')
else:
    # ... rest of the code. runs if the try block succeeded.

Less preferable:

number = input()
if not isinstance(number, int):
    print('Enter a valid integer.')
else:
    # ... rest of the code

Even better, this could be put into a loop to allow the user to retry. It can even be made into a handy function!

def get_user_int(message='Type a number: '):
    while True:
        try:
            print(message)
            return int(input())  # breaks out of the loop
        except ValueError:
            print('Enter a valid integer.')

Than you can safely use get_user_int anywhere in your code. You can pass into it a custom message.

Result

Combining everything I mentioned, here's a possible outcome:

# In this program, the user must try to guess a random number generated by the computer
from random import randint

def guess_game():
    replay = 1   
    # Replay allows a user to replay the game without having to call the function again. 
    # The values are: 1 = Replay, any other character = End
    while replay == 1:       
        lives = get_user_int("How many lives would you like? ")
        max_value = get_user_int("Please choose the largest number that can be generated: ")
        rand_num = randint(0, max_value)
        num_of_guesses = 1  #keeps track of the number of guesses made by the user

        while num_of_guesses <= lives:
            guess = get_user_int("Please enter your guess: ")
            if guess == rand_num:
                print("Good guess, that is correct! You got the right answer on guess number", num_of_guesses, ", well done!\n")
                break
            else:
                print("Unlucky, that guess was incorrect. You have", (lives-num_of_guesses), "lives remaining.\n")
            num_of_guesses += 1

        if num_of_guesses > lives:
            print("You have run out of guesses. The correct answer was:", rand_num)

        replay = int( input("\nWould you like to play again? Enter 1 for yes, any other character for no: ") )
        if replay != 1:
            print("Thank you for playing. See you next time")


def get_user_int(message='Enter a number: '):
    while True:
        try:
            return int(input(message))
        except ValueError:
            print('Enter a valid number.')



guess_game()

Good luck :)

share|improve this answer
    
thank you for your detailed reply. Commenting is still something i am getting to grips with but your explanation about not needing to comment on self-explanatory code is very helpful. Validating user input is a great suggestion, thank you for the example function. i will definitely use it in the future – php123 Jan 15 at 15:05
    
You're welcome :) – Aviv Cohn Jan 15 at 15:14

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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