3
\$\begingroup\$

I've been learning Python for about 4 days now. Python is my first programming language, so I'm not that good at coding yet. I have been programming a little random number game as my first project and came to a state of code with which I'm kind of happy.

My code does what I want: it generates a random number between 1 and 20, prints the initial tip and asks the user for an input guess. Should I guess right it completes the round, adds 100 points to my score and continues with round 2. When I do not guess correctly, it subtracts 20 points off my score and asks me to try again for another 2 attempts until I lose.

In which ways can I improve my code? What are the weaknesses of this code and what could be ways to make code more efficient, clearer and generally better?

import random


attempt = 1
user_input = "ab"


class Test:


    def __init__(self):

        self.score = 100
        self.testguess = 0
        self.y = len("yes")
        self.z = len("no")
        self.number_holder = 0

    def number_gen(self):
        
        self.number_holder = random.randrange(1,20)     

        
    def first_tip(self):
        
        if(self.number_holder % 2 == 0 and self.number_holder <= 10):
            print("*Your number is even and smaller than 10 or the same")
        elif(self.number_holder % 2 == 0 and self.number_holder >= 10):
            print("*Your number is even and bigger than 10 or the same")
        elif(self.number_holder % 2 != 0 and self.number_holder <= 10):
            print("*Your number is uneven and smaller than 10 or the same")
        elif(self.number_holder % 2 != 0 and self.number_holder >= 10):
            print("*Your number is uneven and bigger than 10 or the same")
        
        print("*************************************")
        print(f"*{self.number_holder}")


    def make_try(self):
        
        print("*************************************")
        print("*My guess would be:")
        print("*************************************")
        self.testguess = int(input("*")) 
                    
            
    def check_win(self, attempt):

        if(self.number_holder == self.testguess):
                       
            self.score = self.score
            print("*************************************")
            print(f"*You have won, your final score was: {self.score}")
            print("*************************************")
            return True
            
        
        elif(self.number_holder != self.testguess and self.testguess != 0): 
            
            attempt_calc = 3 - attempt
            self.score = self.score - 20
            print("*************************************")
            print(f"*That was not correct. Your new score: {self.score}")
            print("*************************************")
            print(f"*You have {attempt_calc} attempts left.")
            return False
            

    def start_again(self, user_input, attempt):
        
        x = len(user_input)

        if(x == self.y):
            return True

     
        elif(x == self.z):

            return False
        
        else:
            print("*************************************")
            print("*Wrong input")
            print("*Do you want to play the NumberGame?")
            print("*Type Yes/yes or No/no")
            print("*************************************")
            user_input = input("*")



if __name__ == "__main__":


    T1 = Test()   
 
    for i in range(3):

        print(i)
        
        T1.number_gen()

        while attempt <= 3 and user_input != "no" and user_input != "No":

            
    
            if(attempt == 1):
                print("*************************************")
                print("*Do you want to play the NumberGame?[Yes/No]")
                print("*************************************")
                user_input = input("*")
                ("*************************************")
                
            
            elif(attempt == 2):
                print("*************************************")
                print("*Do you want to try again?[Yes/No]")
                print("*************************************")
                user_input = input("*")
                print("*************************************")
            
            elif(attempt == 3):
                print("*************************************")
                print("*Do you want to try again?[Yes/No]")
                print("*************************************")
                user_input = input("*")
                print("*************************************")
                
                
            if(T1.start_again(user_input, attempt) == False):
                print("*************************************")
                print("*See you next Time!")
                print("*************************************")
                break
    
            if(attempt == 1):
                T1.first_tip()
    
            T1.make_try()
    
            if(T1.check_win(attempt) == True):
                break
                   
            elif(attempt == 3):
                print("*************************************")
                print("*No more attempts.")
                print("*************************************")
                break
            
            attempt = attempt + 1

            print("*************************************")
            print("*Thanks for playing")
            print("*************************************")
        
        attempt = 1
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$
  1. Do not call your class Test - it does not perform tests, does it?
  2. You are initializing some variables (notably, attempt) outside of the class responsible for handling the game state. Make this logic cohesive: the game state should have clear interfaces (taking a guess, trying again...). And the way you are initializing user_input makes it compare to "no" in your logic. Speaking of which...
  3. self.y = len("yes") is a cardinal sin. Not only it is a very backwards way to assign a variable with nonspecific name a value of 3, it is also a fairly weird way to test the user input for "yes" or "no". A more common way to do so if it is case sensitivity that you are concerned with is something like input().lower() == 'yes' or input().lower() in ['yes', 'y'], if you want to allow even more options.
  4. first_tip could and, arguably, should be streamlined:
parity_string = 'even' if self.number_holder % 2 == 0 else 'odd'
order_string = 'less' if self.number_holder <= 10 else 'greater'
print(f"*Your number is {parity_string} and {order_string} than or equal to 10")

Also, um, it gives away the whole game at the end of the method: print(f"*{self.number_holder}")

  1. check_win has an if and elif clauses; the check self.number_holder != self.testguess is completely redundant in the latter and, more importantly, the control flow becomes a mess there: you do not explicitly tackle the case when elif condition is not satisfied (e.g. user inputs 0), so that it returns None and the main loop continues.

  2. Related to the previous issue: you handle some of the game logic in its own class, and some of it in __main__, while essentially all it does in both cases is reading the user input and giving some responses.

  3. start_again seems to silently make an assumption the user will comply the second time and actually type "yes" or "no". Furthermore, it can and will return None if the user messes up the first time (not to mention these length comparisons I wrote about above), and this branch will have a side effect on the loop condition (user_input will be modified and checked in the user_input != "no" bit) - this makes the overall logic quite opaque. And you are not using the attempt variable in this function at all - why have it there, then?

  4. Minor stylistic issues - parentheses in your if statements are completely redundant, self.score = self.score is hardly a useful statement. Neither is T1.check_win(attempt) == True (getting True from True == True is bit weird: it is already true!).

So, the main direction that needs to be taken to improve this code is to address the control flow and make the logic of what happens under which conditions more transparent. Another thing to keep in mind - you are using the user input for both controlling the flow of the game (retrying) and guessing the numbers. In its current state, it makes little sense to store it at all, but you might want to store previous guesses for whatever reason: keep them separate from the input pertinent to the decisions to keep playing.

A bit cleaned up version:

import random

class Game:
    def __init__(self):
        self.score = 100
        self.remaining_attempts = None
        self.user_guess = None
        self.number_holder = None
        self.generate_state()

        
    def generate_state(self):
        self.remaining_attempts = 3
        self.number_holder = random.randrange(1,20)     

        
    def give_hint(self):
        parity_string = 'even' if self.number_holder % 2 == 0 else 'odd'
        order_string = 'smaller' if self.number_holder <= 10 else 'larger'
        print(f"*Your number is {parity_string} and {order_string} than or equal to 10")


    def user_wants_to_continue(self, bad_input=False):
        if self.remaining_attempts == 3:  # Be careful here! Possibly decouple the current attempt from remaining
            prompt = 'Do you want to play the NumberGame?[Yes/No]' 
        else:
            prompt = 'Do you want to try again?[Yes/No]'
            
        print('*************************************')
        if bad_input:
            print('Sorry, try again')
        print(prompt)
        print('*************************************')
        
        user_input = input("*")
        if user_input.lower() not in ['yes', 'no']:
            return self.user_wants_to_continue(bad_input=True)  # Recursive call beating user into submission
        else:
            return user_input.lower() == 'yes'
        
        
    def make_attempt(self):
        print("*************************************")
        print("*My guess would be:")
        print("*************************************")
        self.user_guess = int(input("*"))
        self.remaining_attempts -= 1
                    
            
    def check_win(self):
        if(self.number_holder == self.user_guess):
            print("*************************************")
            print(f"*Congratulations, you have won! Your final score was: {self.score}")
            print("*************************************")
            return True     
        else: 
            self.score -= 20
            print("*************************************")
            print(f"*That was not correct. Your new score: {self.score}")
            print("*************************************")
            print(f"*You have {self.remaining_attempts} attempts left.")
            return False

    
    def play(self):
        user_input = ''
        for i in range(3):
            self.generate_state()
            while self.user_wants_to_continue():
                if self.remaining_attempts > 0:
                    if self.remaining_attempts == 3:
                        self.give_hint()
                    self.make_attempt()
                    if self.check_win():
                        break
                else:
                    print("*************************************")
                    print("*No more attempts.")
                    print("*************************************")
                    break
            print("*************************************")
            print("*Thanks for playing")
            print("*************************************")

I might have changed the logic a little bit, particularly, the fixed loop of 3 games bothers me. But I was not sure about the original intent there.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you fpr taking the time to review my code :) I will redo my code with your tips in mind and hopefully improve it. \$\endgroup\$ Mar 7 at 13:48
1
\$\begingroup\$

As a beginner, focus your attention on building programs with ordinary functions. You built this guessing game using a class. In the abstract, that might be a sensible approach, but you're a raw beginner -- not just at Python but at coding. And the bread-and-butter of effective coding is learning how to break down a problem into small components, each able to perform a narrowly defined task. The best way to gain such skills is by building programs with simple functions. Adding object-oriented code into the mix at this stage is mostly a distraction and often leads to bloated, over-engineered solutions. It also tends to prolong one of the most common beginner bad habits (a reliance on global variables), with expansive classes (such as your Test) becoming a vehicle to smuggle global information back into the mix.

Start with a pseudo-code plan to play the game once. Your game roughly operates like this:

Does the user want to play?
If no:
    exit with goodbye message

secret = generate random number

for attempt in (1, 2, 3)
    if attempt == 1:
        give hint
    guess = get guess from user
    if guess == secret:
        exit with success message

exit with failure message

Create a top-level rough draft of the code, organized around functions. The next step is to convert that logical plan into functions. Functions should be focused: they should do one thing, not everything. Functions should be data-oriented whenever feasible: take data as arguments, return data as results. And functions should be generalized whenever that's helpful to us: for example, if we know we need to ask the user multiple yes/no questions, try to build that behavior once (in a generalized way) rather than multiple times (each with its own particulars); or, to give an example from your current code, if we want to print a headline-banner message we could write a function taking a message and do the printing of the asterisks in just one spot rather repeating those print statements dozens of times. On the first draft, it's alright to take major shortcuts: for example, we might start by skipping the implementation of the needed helper functions. Here's what the game-playing function might look like:

def number_game():
    if not yesno('Want to play the number game'):
        exit_game('Goodbye')

    secret_range = range(1, 21)
    secret = random.choice(secret_range)

    for attempt in range(1, 4):
        if attempt == 1:
            print(get_hint(secret))
        guess = get_guess(secret_range)
        if guess == secret:
            exit_game('You won')

    exit_game('You lost')

if __name__ == '__main__':
    number_game()

Filling in the gaps: helper functions. So far, we have a function to play a single game, but we are missing the lower-level functions. I'll leave the helper functions to you, but I'll offer one suggestion. People make mistakes so you should give your user the ability to recover from data-entry errors and you should validate their inputs: for example, if you ask for yes/no, make sure you get a 'yes' or a 'no'; or if you ask for an integer from 1 through 20, make sure you get one. The most flexible way to do that is with a while-true loop: get the user response; check it for validity; if OK, return the converted/validated value; otherwise, stay in the while-true loop and ask again.

def yesno(prompt):
    # Return True or False.
    while True:
        ...

def get_guess(secret_range):
    # Return an int in the range.
    while True:
        ...

def get_hint(secret):
    # Return a message.
    ...

def exit_game(message):
    print(message)
    exit()

Filling in the gaps: the ability to play more than once. If you've implemented the ability to play once using well-designed functions, switching to playing multiple times is usually easy. (1) We add a top-level function to ask whether the user wants to play again. (2) We convert number_game() into a data-oriented function that returns true/false rather than a function that exits.

def main():
    while yesno('Want to play the number game'):
        player_won = number_game()
        print('You won' if player_won else 'You lost')
    exit_game('Goodbye')

def number_game():
    secret_range = range(1, 21)
    secret = random.choice(secret_range)
    for attempt in range(1, 4):
        if attempt == 1:
            print(get_hint(secret))
        guess = get_guess(secret_range)
        if guess == secret:
            return True
    return False

if __name__ == '__main__':
    main()

Functions are flexible. As illustrated in the previous point, the approach suggested above is flexible for future enhancements: for example, we could tally up wins/losses and report that at the end of main(); we could take command-line arguments to let the user control the size of secret_range; we could enhance the get_hint() logic to tell the user whether their guess was too low or high. Such flexibility comes from the focus on organizing the program around small data-oriented functions.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ I thank you a lot for for taking the time to review my code :) I will work on it again with your tips in mind. \$\endgroup\$ Mar 7 at 13:41

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.