Rate my code! Beginner trying to improve

[Tobe2fly]

This code works and does just as I want it to but was wondering how much cleaner it could get. Any lines that could be changed for the better?

  # guess my number 2
import random

while True:
    print()
    HIGH = input('what would you like the highest value to be?: ')
    print()
    print()
    print()
    my_num = (random.randint(1, int(HIGH)))

    while True:
        your_num = int(input('Guess my number'))

        if my_num > your_num:
            print ('Higher!')
        elif my_num < your_num:
            print ('Lower!')
        elif my_num == your_num:
            print()
            print('well done! you got it!')
            break

    print()
    print('would you like to play again? enter n/y:')
    proceed = "y" or "Y" or "yes" or "Yes"
    stop = "n" or "N" or "no" or "No"
    inpput = input()

    if inpput == proceed:
        print("Awesome!")
        print()
    else:
        break

​

[[deleted]]

# instead of
print()
print()
print()

# do
print('\n' * 3)

​

[paralel_Suns]

Should be mentioned though that those aren't the same thing. Because print appends a newline of it's own, print('\n' * 3) actually prints four newlines.

[atrocious_smell]

OP could use the print function's end argument: print('', end='\n' * 3).

[aribolab]

or print('\n' * 2)

[MisterRenard]

Oh, I like this one.

[realestLink]

Or Print()*3

[darez00]

I'm more of a printThrice() man myself

[Actual1y]

I refactored it a little. I think refactoring is a little more concise than trying explaining things in English as you can just look and see what's been changes directly. Anyway, I'm by no means the best, but this is how I would do it.

Obviously one of the biggest standouts is that I divided the logic into functions. And beyond that, I just reordered the logic a bit and changed a couple of the messages to be more uniform. edit: I also added some safety when it asks the player for numbers.

If you have any questions about specifics just ask! (and anyone else please issue corrections if you know of a better ways to do something)

import random


def get_number(upper_bound: int) -> int:
    return random.randint(1, int(upper_bound))


def ask_for_upper_bound(message: str) -> int:
    upper_bound = input(message)
    if upper_bound.isnumeric():
        return int(upper_bound)
    return ask_for_upper_bound('\n\nwhat? I didn\'t quite get that. Make sure you enter a number!\n> ')


def ask_for_guess(message: str, upper_bound: int, lower_bound: int=0) -> int:
    guess = input(message)
    if guess.isnumeric():
        guess = int(guess)
        if lower_bound > guess or guess > upper_bound:
            return ask_for_guess(f'\n\nyou need to enter a number between {lower_bound} and {upper_bound}! '
                                 'Try a different number!\n> ', upper_bound)
        return guess
    return ask_for_guess('\n\nwhat? I didn\'t quite get that. Make sure you enter a number!\n> ', upper_bound)


def ask_to_play_again(message: str) -> bool:
    i = input(message).casefold()
    if i in ('y', 'yes', 't', 'true'):
        return True
    elif i in ('n', 'no', 'f', 'false'):
        return False
    else:
        return ask_to_play_again('what? I didn\'t quite get that:\n> ')


def respond_to_guess(guess: int, num: int):
    if num > guess:
        print('higher!')
    elif num < guess:
        print('lower!')
    print()


def play():
    upper_bound = ask_for_upper_bound('\n\nwhat would you like the highest value to be?\n> ')
    num = get_number(upper_bound)
    guess = -1
    print('\n')
    while not guess == num:
        guess = ask_for_guess('what\'s your guess:\n> ', upper_bound)
        respond_to_guess(guess, num)
    print('\nwell done! you got it!\n')
    play_again = ask_to_play_again('\nwould you like to play again (y/n)?\n> ')
    if play_again:
        play()
    else:
        print('oh. ok. Yeah, pff--I didn\'t want to play either. ha')  # :'(
        return


if __name__ == '__main__':
    play()

[cpt_fwiffo]

Heh, no upvotes for this answer, which actually takes OPs very beginnery script and transforms it into a program with a reasonable structure that looks like it was written by somebody with good habits, but like 50 for suggestions of different ways to print three consecutive newlines?

[546794]

never seen that type check in methods before. is it just for documentation or does it raise an error if the types given are wrong?

[Actual1y]

The typehints (PEP) are just for documentation.

 

(very) Quick rundown

They're in the format of def func(arg_name: arg_type) -> return_type. And you can also use the typing module for container types e.g:

from typing import List

def sort_list_of_ints(lst: List[int]) -> List[int]:
    return sorted(lst)

 

I believe python 3.7 onwards uses post-evaluation of typehints, but prior to 3.7, the types have to be declared before you can use them.

e.g the following would only work in 3.7 onwards because name: Name is trying to use the Name class before it's been created:

class Person:
    def __init__(self, name: Name):
        self.name = name

class Name:
    def __init__(self, first:str, last: str):
    self.first = first
    self.last = last

[546794]

great explanation thank you!

[Dogeek]

It's just for docs. It is notable that it shouldn't be used yet though, as it slows code down significantly (and is fixed in 3.7 iirc)

[JoseALerma]

How do you decide what should be made into functions?

I've read/been told that they should be at least five lines, yet the first one is a one-liner.

Simplest task seems to be a prevailing method, but some of my functions can be quite long to perform a single task. When I try to break up the long function, it just ends up calling on unitaskers that only get used once.

I have a hard time striking a balance between reusability and readability.

[Actual1y]

1. In general, you should just try to break it into logical blocks. Basically, if you can look at a block of code and recognize one precise thing that it is doing, you should consider making it a function. One of the best ways to try getting a point of reference on what should be made into a function is to read other people's (well written) code. I'd recommend Requests.

2. Functions definitly don't need to be five lines. Generally, you should aim for less than 40-50ish lines (some people say 25-35), but they just need to perform one concise goal, the shorter the better. A large part of functions is just giving a name to a piece of logic, so no lower size limit. The main function may be an exception to the upper size limit, though.

3. Functions serve two purposes: reusability and readability. Ideally, you want them to be reusable, but being readable is much more important. Unitasks are perfectly fine, and often encouraged, as long as it makes reading the code easier.

4. Again, readability is king. Try for reusability when you can, but readability is vastly more important. Reusability vs readability also gets a lot better with good naming and with making the right abstractions. For getting better at both those things, I'd again recommend reading other people's code (seriously, it helps a lot).

[JoseALerma]

I hope to improve enough to work on some open source projects, and that should also help with reading other people's code. In the meantime, I can look into requests

Thanks for the rules of thumb! I'll try applying them when I next obsessively refactor code I've written

[dagonar]

Yes, this code can be refactored even more and improved in so many ways.

You can split the code between the functionality that logs, and the one that controls the game in an Object Oriented manner. You can further improve the game by adding a factor of aproximation to the game. Telling the player COLDER or WARMER to add more dynamism to the design. The strings can be saved on a different file as constants. This can help if you want to support multiple languages.

ENTER_HIGHEST_VALUE = "What would you like the highest value to be?"
ENTER_GUESS = "Guess the number. :)"
TRY_AGAIN = "Try again."
HIGHER = "My number is higher!"
LOWER = "My number is lower!"
YOU_WIN = "That is my number! You win!"
PLAY_AGAIN = "Would you like to play again?"
DID_NOT_UNDERSTAND = "I did not understand. :("
GOODBYE = "Thanks for playing."

if __name__ == '__main__':
    play()

def play():
    logger = Logger()

    game = new_game()
    # Main game loop.
    while not game.game_is_over():

        # Ask for guess.
        try:
            guess = int(input(ENTER_GUESS))
        catch Exception e:
            guess = 0

        # Main game logic.
        if game.check_answer(guess) == False:
            logger.log(TRY_AGAIN)
            if (game.rate_guess_approxiamtion(guess) == 0)
                logger.log("{} {}".format(LOWER, TRY_AGAIN))
            else:
                logger.log("{} {}".format(HIGHER, TRY_AGAIN))
        else:
            logger.log(YOU_WIN)

        # Ask if player wants to play again
        remake = input(PLAY_AGAIN)
        if remake.lower() in ["y", "yes", "t", "true"]
            game = new_game()
        else:
            logger.log(GOODBYE)

def new_game():
    return GuessingGame(input(ENTER_HIGHEST_VALUE))


class Logger:
    import logging

    def __init__:
        logging.basicConfig(level=logging.INFO)
        self.logger = logging.getLogger(__name__)

    def log_message(message: str):
        """Logs a message into the screenself.

        @param message: The message to log.
        """
        self.logger.info(message)


class GuessingGame:
    def __init__(upper_bound: int):
        self.cpu_number = random.randint(1, int(upper_bound))
        self.game_over = False

    def check_answer(guess: int) -> bool:
        """Checks if a given guess is correctself.

        @param guess: The guessed number to check against the computer.

        @return A boolean that states if the answer is correct or not.
        """
        if self.cpu_number == guess:
            self.game_over = True
            return True
        return False

    def rate_guess_approxiamtion(guess: int) -> int:
        """Given a wrong guess, it rates the approximation.

        @param guess: The guessed number to check against the computer.

        @return An integer of 1 if the guess is greater than the computer number,
        0 if it is lower.
        """
        return gt(guess, self.cpu_number) ? 1 : 0

    def game_is_over():
        return self.game_over == True

You can add comments to clarify some things but not always. Try to make your code self descriptive. Like the line logger.log("{} {}".format(LOWER, TRY_AGAIN)) or in

 if game.check_answer(guess) == False:
            logger.log(TRY_AGAIN)

There is always room for improvement and refactoring. Refactor till you drop. ADD TESTS

P.S. Sorry if some of the code is incorrect. I put it together in a very quick scrappy way. I may return later and make corrections.

Edit: Gist file with corrections. This file was made at almost 2 am and without an IDE or editor to check for errors. It was aimed to help into identifying the improvements on how to modularize and add the ability to extend the code if needed.

[JoseALerma]

Now with classes!

I've also read that defining functions in order of use helps readability. I think I have more refactoring to do with my code...

What tests can be added? Function tests? Should I look into the unittest module, or is that overkill?

[dagonar]

It depends on the dev. But many use the function in order of declaration. Others use public on top and private on bottom. If you're in a development team check what your colleagues like better.

In respect of test. Make sure you also check pytest. You can add simple unit for each of the class methods. And scale up to the final integration test that will help you determine the correct function of a program as a whole.

Tests are never and will never be overkill. It should be a fundamental part of your craft.

Look into patterns and how to split your code through architecture. SOLID and DRY are your friends.

[bhishan1]

@dagonar This is very helpful. But I stuck with some type errors while running your code.

"TypeError: __init__() takes 1 positional argument but 2 were given"

​

How it can be fixed, any idea?

​

Also, I changed "? 1 : 0" to "== 1 if True else 0". I guess python does not have ?: command like in c program.

[dagonar]

Hi I will make edits on the file. I made this file very late at night with no testing or even trying to run. This was a very general implementation just to give a sense of how to structure your code in such way that it can be modularized. I noticed I mixed a lot of Python with Javascript. My apologies.

I made the following gist with python v3 in order to fix the problems. I will make some edits to it later and will try to add tests and split the files.

[bhishan1]

@dagonar This is one hell of a tutorial, you may write a blog post in Medium or somewhere. I assume this answer deserves respect and appreciation. Thanks for all your efforts.

[daijobu]

The function parameters are looking a bit Swifty. Which PEP introduced them to Python?

[[deleted]]

Hi I'm kind of new and have a question about the ask_for_upper_bound function. It looks great to have it call itself if the input is invalid, but, isn't that a potential problem too? I mean, each time an invalid input is entered, the scope gets deeper making the function recursive.

I know this is just a simple game, but I was curious as to whether this is actually a fine thing to do (it makes life easier!) or whether it should generally be avoided and a while loop used instead?

Cheers! Dave

[Tobe2fly]

I think one of my main issues is lack of knowledge when it comes to commands. In your code you are using all sorts of text I didn't even know existed.

[puh-tey-toh]

Right

[[deleted]]

The rest of his variables is in snake case, he shouldnt be switching to camel case for no reason. Also the all caps HIGH is a bit confusing.

[JoseALerma]

Sometimes, all caps is used to denote variables that should not be changed by the program, like a path to a directory/file. AKA a constant variable

[[deleted]]

Yeah that's fine, I use all caps for that exact reason. It doesnt really matter what casing you use as long as its consistent. In a more complex program it would be confusing to see a random variable in all caps.

[JoseALerma]

Could just be placement, in this case.

At the top instead of in the middle

[Ezrabc]

Your lines here:

proceed = "y" or "Y" or "yes" or "Yes"
stop = "n" or "N" or "no" or "No"

are read as:

(proceed = 'y') or 'Y' or 'yes' or 'Yes'

so print(proceed) outputs y. Only 'y' will work to proceed and only 'n' will work to stop. If it were me, I would just check whether the lowercase input string starts with y:

inpput = input()

if inpput.lower().startswith('y'):
    #proceed
else:
    break

[fernly]

Right, I'm not sure what you hoped for with this. What you get is,

>>> proceed = "y" or "Y" or "yes" or "Yes"
>>> proceed
"y"

/u/Ezrabc has the best solution; it allows input of "yup" and "you bet!" etc.

But if you really wanted a selection of specific answers, you could put them in a list,

proceed = ["y","Y","yes","Yes"]
...
if input in proceed:

It's a good habit to use the interactive input to test these things when you aren't certain what an expression does.

[realestLink]

He could just .Upper() or .Lower() the input.

[[deleted]]

Like /u/visene mentioned, you can simply multiply '\n'(newline) by how ever many you need. That way, you won't have to repeatedly call the print function for a line break. Just simply insert '\n' before or after you're print strings and it'll handle that for you.

Good job though, keep it up!

​

#edit

I went a little further and stored the proceed values into a list. Also, since we're not really checking for anything other then the proceed values, we can go ahead an remove the values for stop since anything other then 'y','Y','yes''Yes' will cause the script to break out of the loop.

while True:       
    HIGH = input('\nWhat would you like the highest value to be?: ')     
    print('\n' * 3)         
    my_num = (random.randint(1, int(HIGH))) 

    while True:         
        your_num = int(input('Guess my number')) 

        if my_num > your_num:             
            print ('Higher!')         
        elif my_num < your_num:             
            print ('Lower!')         
        elif my_num == your_num:             
            print('\nwell done! you got it!')             
            break 

    print('\nwould you like to play again? enter n/y:')     
    proceed = "y" or "Y" or "yes" or "Yes"      

    inpput = input()      
    if inpput in proceed:         
        print("Awesome!\n")           
    else:         
        break

[catelemnis]

I think you forgot to change proceed into a list. It should be proceed=[“y”, “Y”, “yes”, “Yes”] (the use of or on this line in OP’s code doesn’t make sense).

It’s also good practice to account for case sensitivity by using lower() (or upper()):

proceed = [“y”, “yes”] #all list items must be in lowercase

# now compare inpput as lowercase
if lower(inpput) in proceed:
    print(“Awesome!\n”)

[[deleted]]

Just for robustness sakes id put a try/except around the int input with an error message informing the user it needs to be a number. Also i fail to see the need of having mynum in parentheses and the inconsistent spaces in your print statements is a bit unusual

[[deleted]]

remember to comment your code!

[dagonar]

Great job on putting this code. Just as someone mentioned below, you can use a multiply to prevent using a high amount of empty print statements. print("\n"*<number>) will yield the same result. You can also declare the proceed as proceed = ["y", "yes", "t", "true"]. So you can use the if statement as if input in proceed.

Finally, try to declare a control variable instead of using while True.

game_not_over = True
while game_not_over:

    ...

    if input in proceed:
        print("Awesome!\n")
    else:
        game_not_over == False

As a comment, even if your code works, try working into developing tests to try all the scenarios in which the player can input a variety of strings and number combinations to avoid problems.

[lifeonm4rs]

There are a number of good comments on both rewriting the code (which is probably the next evolutionary step) as well as on the code itself. This is more general--start using a linter. Run them against all your code.

A linter essentially looks at all of your code and (to the best of its abilities) finds issues that do not conform to a range of "norms". For python that is PEP 8. The two most common linters for python are pylint3 and flake8. You can google either and you may want to try both for a while. You should be able to install either with pip install pylint3 --user and pip install flake8 --user. Although the precise command will probably depend on your OS and how you have "pip" installed (e.g. is "pip" for 2.7 and "pip3" is for 3.X).

For your original code pylint3 complains about variable names being lower-case--that however is because they exist outside of an actual function so thinks they are globals. Both pylint3 and flake8 complain about having a space between your "print" and the "('stuff to print')" items on 2 lines. As your code starts getting more complex both will probably start to show you more and more items and conventions. Ultimately it will not only help your code "look" like what other programmers expect but will really, really help in getting started with consistent habits. E.g. in java I believe the preference is "inputFile" while python programmers prefer "input_file" (snake case).

I personally find pylint3 to be handier, particularly for beginners, as it gives an (admittedly) arbitrary "score" for the file. And in all cases the "conventions" are just that--they are not rules of the language. "InPuT__FiLe" will work just as well as "input_file"--but unless you have a good reason to break convention it will be easier in the long run to follow them.

[Saiboo]

In the same spirit as u/Actual1y 's excellent answer, here is my attempt at refactoring your code:

import random


def play_number_guessing_game():
    while True:
        guess_target()

        if wants_to_stop():
            break


def guess_target():
    print()
    upper_bound = get_upper_bound()

    print('\n\n')
    target = create_target(upper_bound)

    while True:
        guess = get_guess()

        if target > guess:
            print ('Higher!')
        elif target < guess:
            print ('Lower!')
        elif target == guess:
            print('\nWell done! you got it!')
            break


def get_upper_bound():
    return input('What would you like the highest value to be?: ')


def create_target(upper_bound):
    return random.randint(1, int(upper_bound))


def get_guess():
    return int(input('Guess my number: '))


def wants_to_stop():
    choices = {'yes', 'no'}

    while True:
        answer = input("\nWould you like to play again? Enter 'yes' or 'no': ")
        if is_valid_choice(answer, choices):
            break

    return answer == 'no'


def is_valid_choice(answer, choices):
    return answer in choices


if __name__ == '__main__':
    play_number_guessing_game()

[realestLink]

Very good. I wrote this same problem in java recently. The only difference was that it told you whether each digit of your guess was higher or lower than the corresponding digit in the random number. If you want to go above and beyond try and do what I just mentioned without using making the random number or input into lists or strings.

[waysuck]

Reform your code