After the feedback for my last script, I realized I should be using functions and trying to adhere to the style guide. I tried to do that here, though still very beginner.
The rounding in Python confused me a bit, and originally the program got stuck for some numbers when the range narrowed down to 1. I think it should work for all cases now (only tested 1-1000).
# Users chooses number in given range, comp guesses
HIGH_NO = 10000
LOW_NO = 1
def getno():
# Get a number in range from user.
x = ""
while not x or x > HIGH_NO or x < LOW_NO:
print("Enter a number between %s and %s" % (LOW_NO, HIGH_NO))
x = input()
try:
x = int(x)
except ValueError:
x = getno()
return(x)
def approxhalf(lowno, highno):
# Find integer approx halfway point between two values.
x = abs(round((lowno+highno)/2))
return x
def guessno():
# Computer finds number by halving
no1 = LOW_NO
no2 = HIGH_NO
userno = getno()
compguess = ""
count = 0
while compguess != userno:
count += 1
compguess = approxhalf(no1, no2)
if abs(no2-no1) > 2 or compguess == userno:
print('%s. I guess %s' % (count, compguess))
if compguess > userno:
no2 = compguess
elif compguess < userno:
no1 = compguess
elif compguess == userno:
print("Got it!")
else:
print("wtf dude")
else:
# For when the low and high no.s are consecutive
if no1 == userno:
print('%s. I guess %s' % (count, no1))
print("Got it")
break
elif no2 == userno:
print('%s. I guess %s' % (count+1, no2))
print("Got it")
break
else:
print("I.. I don't understand")
def main():
while True:
guessno()
print('\n')
main()
3 Answers 3
getno
Python lets you assign values to variables without regard for type, but you really shouldn't abuse that capability. Changing the type makes it hard to reason with the code. Here, x
starts off as a string, then becomes an int. The problem is, what does not x
mean? For strings, it means "x
is empty"; for numbers, it means "x
is zero". Due to that confusion, you've introduced a bug: 0 will always be accepted, even if it falls below LOW_NO
. (You have committed a similar sin in guessno()
by initializing compguess = ""
.)
Some other faults, going line by line:
- As mentioned by other users,
getno
is a rather unfortunate name, since it looks like "no" as in yes/no. - The comment should be a docstring.
- You should use double-ended inequalities.
- A
"%d"
placeholder would be clearer than"%s"
. However,str.format()
would be even more preferred in modern Python code. input()
can print a prompt.- The use of recursion is inappropriate, especially considering that you already have a
while
loop. - There is no clear error message when the user gives unacceptable input. It would be especially frustrating if the user enters a non-integral number within the range.
- Conventionally,
return
statements shouldn't have parentheses. It's visual clutter, and it also looks like a function call.
Here's how I would write it:
def ask_int(lo, hi):
"""Ask the user for an integer in the inclusive range."""
prompt = 'Enter a number between {0} and {1}: '.format(lo, hi)
while True:
try:
n = int(input(prompt))
if lo <= n <= hi:
return n
else:
print('Input is out of range.')
except ValueError:
print('Input must be an integer.')
approxhalf
- I suggest renaming it to
approx_mid(lo, hi)
. - Docstring
- What's the
abs()
call for? - You can skip the assignment to
x
.
guessno
There are comparisons all over the place: in the while
test, the if
, and the innermost if-elif
chains. There are also three "Got it"
cases, where ideally there should be just one. I would reorganize the code like this:
from itertools import count
def guess(lo, hi):
user_num = ask_int(lo, hi)
for guess_count in count(1):
comp_num = approx_mid(lo, hi)
print('{0}. I guess {1}'.format(guess_count, comp_num))
if comp_num == user_num:
print('Got it!')
return
elif comp_num == lo:
# Shouldn't happen, if approx_mid() rounds up
lo = hi
elif comp_num == hi:
# approx_mid() should only guess hi if lo and hi are consecutive
assert hi - lo == 1
hi = lo
elif comp_num < user_num:
lo = comp_num
elif comp_num > user_num:
hi = comp_num
else:
assert False
The "wtf" and "I don't understand" cases should be assertions. Swearing in comments is one thing, but you shouldn't write obscenities that have any chance of being exposed to the user.
General logic and flow
I recommend removing not x
and instead just originally setting x = LOW_NO-1
for your while
x = LOW_NO - 1
while HIGH_NO < x or x < LOW_NO:
Next, I recommend you just keep the while
loop instead of recursively calling getno
if the user enters and invalid int
. You could just use pass
to tell the loop to skip onto the next loop. The old value of x
will be preserved so you the condition is still true. Though I do recommend informing the user why their input was rejected.
try:
x = int(x)
except ValueError:
print ("That's not a valid number")
Lastly, just return x
, no need for brackets.
In your approxhalf
, there's no need for abs
. Your LOW_NO
and HIGH_NO
values should prevent wildly off numbers, and if you ever want to use negative numbers there's no need to prevent it like this. I do recommend using int()
here just so that it prints without decimal places, since the actual result is based on whole numbers. You also don't need to assign x
just to return
it. Return the value directly.
def approxhalf(lowno, highno):
# Find integer approx halfway point between two values.
return int(round((lowno+highno)/2))
The reason rounding has confused you is probably to do with how floats work. For example 0.1
is actually 0.1000000000000000055511151231257827021181583404541015625
. This can affect rounding in cases where you think a number is x.5
but it's actually represented in memory as x.49942342341
, leading to a number that rounds down instead of up.
In your checking system, I think you should rearrange the flow.
It's confusing to check if compguess
is right and at the same time check if it's more than 2 off. You should do each distinct condition as it comes, my preference is to start with the correct answer and broaden out from there:
if compguess == userno:
print('%s. I guess %s' % (count, compguess))
print("Got it!")
break
A note, you used while compguess != userno
. But you break out of the loop for other conditions later down. I'd recommend instead using while True
and then use break
for the various points you actually want the loop to be over.
if abs(no2-no1) <= 2:
# For when the low and high numbers are consecutive
if no1 == userno:
print('%s. I guess %s' % (count, no1))
print("Got it")
elif no2 == userno:
print('%s. I guess %s' % (count, no2))
print("Got it")
else:
print("ERROR: Neither %s nor %s == %s" % (no1, no2, userno))
break
About that last else. If you're going to have a condition for a seemingly impossible situation, then you should at least output some relevant data to understand what went wrong. Also I put the break outside the if
branches because it should break regardless of which condition is True
. Note that now we don't need an if for the next part, we know it has to evaluate since no other condition (which would end the loop) was True
.
if compguess > userno:
no2 = compguess
elif compguess < userno:
no1 = compguess
Style advice
Reading PEP0008 will tell you all about what I'm saying here and more. It's highly recommended reading.
You should name your functions using underscores, as that's the recommendation. So you'd have get_no()
, guess_no()
and approx_half()
. As for variable names, CamelCase is better for compGuess
and userNo
.
You almost but not quite have docstrings. You have comments explaining what each function does but formatting as an actual docstring has a practical usage.
def approx_half(lowno, highno):
""" Find integer approx halfway point between two values."""
return int(round((lowno+highno)/2))
Enclosing the text in triple quotes when it's at the start of a function will make it a docstring, which means that people can access it in code or in the interpreter to investigate your description of it.
>>> help(approx_half)
Help on function approx_half in module __main__:
approxhalf(lowno, highno)
Find integer approx halfway point between two values.
Good overall. Some suggestions:
- In
getno
you are using a while loop and recursion. I would use one or the other. In your case, if you get aValueError
I would print a message then setx = 0
, then just continue the loop as-is. - In
getno
if you set the defaultx
to0
, and combine that with my previous suggestion, you can get rid of thenot x
test. return
isn't a function, it doesn't need parentheses.- I would make the low and high limits arguments in all cases, which default to using the global values.
- I would make
compguess
default to0
. Needlessly mixing types makes the code more brittle. It works now, but changes later can inadvertently introduce bugs. - In
guessno
, you can flatten theif
test to a single level. First check if the guess is correct, then check ifno1
is right, then check ifno2
is right, then check if the numbers are consecutive, then the guess is high, then check if the guess is low. - The loop should terminate, or better yet raise an exception, if there is a nonsensical result. It currently just keeps running.
- The
while
test should probably just run forever, andbreak
out of it when it is done. - You should only run
main()
if__name__ == '__main__'
. In other words, only runmain()
automatically if the file is run as a script.
number
everywhere you haveno
.no
is a very easily misread abbreviation. \$\endgroup\$num
. \$\endgroup\$userno
of 1 should have acount
that is 1 higher than thecount
for auserno
of 2, but as written, whenno1=1
andno2=3
, what the computer guesses with the nextcount
will differ, depending on the value ofuserno
. \$\endgroup\$