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 randomattempt = 1user_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
Reinderien's user avatar
Reinderien
71.2k5 gold badges76 silver badges256 bronze badges
askedMar 4, 2022 at 7:36
Marc Matthäus's user avatar
\$\endgroup\$

2 Answers2

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 initializinguser_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 likeinput().lower() == 'yes' orinput().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 anif andelif clauses; the checkself.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 returnNone 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 theuser_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 isT1.check_win(attempt) == True (gettingTrue fromTrue == True is bit weird: it isalready 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 randomclass 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.

answeredMar 4, 2022 at 14:51
Lodinn's user avatar
\$\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\$CommentedMar 7, 2022 at 13:48
1
\$\begingroup\$

As a beginner, focus your attention on building programs with ordinaryfunctions. You built this guessing game using a class. In the abstract, thatmight be a sensible approach, but you're a raw beginner -- not just at Pythonbut at coding. And the bread-and-butter of effective coding is learning how tobreak down a problem into small components, each able to perform a narrowlydefined task. The best way to gain such skills is by building programs withsimple functions. Adding object-oriented code into the mix at this stage ismostly a distraction and often leads to bloated, over-engineered solutions. Italso tends to prolong one of the most common beginner bad habits (a reliance onglobal variables), with expansive classes (such as yourTest) becoming avehicle to smuggle global information back into the mix.

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

Does the user want to play?If no:    exit with goodbye messagesecret = generate random numberfor attempt in (1, 2, 3)    if attempt == 1:        give hint    guess = get guess from user    if guess == secret:        exit with success messageexit with failure message

Create a top-level rough draft of the code, organized around functions. Thenext step is to convert that logical plan into functions. Functions should befocused: they should do one thing, not everything. Functions should bedata-oriented whenever feasible: take data as arguments, return data asresults. And functions should be generalized whenever that's helpful to us: forexample, if we know we need to ask the user multiple yes/no questions, try tobuild that behavior once (in a generalized way) rather than multiple times(each with its own particulars); or, to give an example fromyour current code, if we want to print a headline-banner messagewe could write a function taking a message and do the printingof the asterisks in just one spot rather repeating those printstatements dozens of times.On the first draft, it's alright to take majorshortcuts: for example, we might start by skipping the implementation of theneeded 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 asingle game, but we are missing the lower-level functions. I'll leave the helper functions to you, but I'lloffer one suggestion. People make mistakes so you should give your user theability 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 ifyou ask for an integer from 1 through 20, make sure you get one. The mostflexible way to do that is with a while-true loop: get the user response; checkit for validity; if OK, return the converted/validated value; otherwise, stayin 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'veimplemented the ability to play once using well-designed functions, switchingto playing multiple times is usually easy. (1) We add a top-level function toask whether the user wants to play again. (2) We convertnumber_game() into adata-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 Falseif __name__ == '__main__':    main()

Functions are flexible. As illustrated inthe previous point, the approach suggested above is flexible for future enhancements: for example, we could tally upwins/losses and report that at the end ofmain(); we could take command-linearguments to let the user control the size ofsecret_range; wecould enhance theget_hint() logic to tell the user whether theirguess was too low or high. Such flexibility comes from the focuson organizing the program around small data-oriented functions.

answeredMar 4, 2022 at 19:30
FMc's user avatar
\$\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\$CommentedMar 7, 2022 at 13:41

You mustlog in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.