2
\$\begingroup\$

I am new to programming, and I wanted to ask, is my code inefficient / poorly written?

Here is my code below for an exercise I just did. The point is for the user to guess the number 0, but it also:

  • Calculates the average number
  • Calculates the number of attempts
  • Gives the sum of attempts

Are there ways of improving this code? If so, how do you know where to look? Am I overusing if/while statements?

#User must guess a number: until they reach value of 0
#Must calculate average sum of guesses, track sum of numbers entered, keep count of attempts

guess = ''
count = 0
total_sum = 0 

while guess != 0:
    print("Guess the number: ")
    guess = int(input())

#Count no. of attempts
    count += 1

#Total Sum of guesses
    total_sum += guess

    if guess == 0:
        print("Congratulations!, you guessed the correct number")

    #Average number of guesses
        try:
            average = (total_sum / (count - 1))
            print("Average number:", average)
        except ZeroDivisionError:
            print("No average")

    print("Attempts: ", count)
    print("Total sum of guesses: ", total_sum)
\$\endgroup\$
  • 2
    \$\begingroup\$ To know if something is inefficient you run it and if it takes too long to run, then that's normally a hint. From there you profile your code and determine the complexity of the offending code, to see if you can get something better. \$\endgroup\$ – Peilonrayz Mar 3 at 14:25
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ – Toby Speight Mar 3 at 14:28
3
\$\begingroup\$

Well, I always start with using a main block. It's just a good habit to have.

Name your constants. What if you wanted the change the number the user was supposed to guess to, say, 5? You already have at least two places in the code where 0 shows up, and what you actually mean by that is "the target value". Give it a name and use that name.

target = 0

while guess != target: # ...

You're checking the game-end logic twice, once in the outer while loop, once in the inner if check. Only do so in one place. E.g., make the outer loop while True: or, better yet, for _ in range(max_num_guesses): with an explicitly set maximum number of guesses. Then, inside the if statement that checks if the user got the answer right, just break to exit the loop.

Now, about what you particularly ask about: computing the sum/mean/count of the users' guesses. Your current approach scatters the logic of what you're trying to do: summation happens on one line, counting on another, division on another... this code sample isn't big, but already this goal is scattered across your code. It'd be a lot cleaner if we dealt with all those similar things in one place. To do this, let's just keep track of the guesses inside the loop, then later, we'll do all the relevant computations with them. Something like:

target = 0
guesses = []

while True:
    print("Guess the number: ")
    guess = int(input())
    guesses.append(guess)

    if guess == target:
        break

    print("Nope.")

print(f"Attempts: {len(guesses)}")
print(f"Total sum of guesses: {sum(guesses)}")
print(f"Average guess: {sum(guesses) / len(guesses)}")

This snippet doesn't do exactly what your code does, but hopefully it gives you an idea for how you can keep different chunks of your code focused on different purposes, and thus keep each bit clean and keep the logic simple. See how the while-loop section is solely focused on running the game logic, and the print section is solely focused on what details you want to print back to the user. The only crossover is the the guesses.append(guess) line in the loop, which just says "I'll want this info later" and let you, later, figure out exactly what you want to do. E.g., let's say you wanted to report the median guess, or how many guesses were above versus below the target; these are easy to add later if we have a list of guesses, but would add yet more fluff to the loop if we tried to keep track as we went along. Note that, since you're just keeping track of user input, this list is never expected to get so big that counting and summing it is worth bothering about; even if you want to report this value in each loop iteration, just compute it each time rather than trying to calculate things incrementally.

Oh, and finally, while I'm all for the try-and-fail when it makes your code cleaner, this probably isn't such an occasion. I'd consider:

if count > 1:
    average = (total_sum / (count - 1))
    print("Average number:", average)
else:
    print("No average")

or even:

print(f"Average number: {total_sum / (count - 1)}" if count > 1 else "No average")

to be cleaner than using a try/except ZeroDivisionError here.

(PS: Note the use of f-strings rather than relying on the print function to append things to strings... that's also a good habit, though not a significant improvement in this particular case.)

| improve this answer | |
\$\endgroup\$

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.