8
\$\begingroup\$

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()
\$\endgroup\$
3
  • 3
    \$\begingroup\$ I highly suggest using number everywhere you have no. no is a very easily misread abbreviation. \$\endgroup\$ – Schism Aug 19 '15 at 18:43
  • 5
    \$\begingroup\$ At least use num. \$\endgroup\$ – Panzercrisis Aug 19 '15 at 20:24
  • \$\begingroup\$ Computer's last guess is psychic. A userno of 1 should have a count that is 1 higher than the count for a userno of 2, but as written, when no1=1 and no2=3, what the computer guesses with the next count will differ, depending on the value of userno. \$\endgroup\$ – Dan Henderson Aug 20 '15 at 17:23
3
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.
\$\endgroup\$
0
5
\$\begingroup\$

Good overall. Some suggestions:

  1. In getno you are using a while loop and recursion. I would use one or the other. In your case, if you get a ValueError I would print a message then set x = 0, then just continue the loop as-is.
  2. In getno if you set the default x to 0, and combine that with my previous suggestion, you can get rid of the not x test.
  3. return isn't a function, it doesn't need parentheses.
  4. I would make the low and high limits arguments in all cases, which default to using the global values.
  5. I would make compguess default to 0. Needlessly mixing types makes the code more brittle. It works now, but changes later can inadvertently introduce bugs.
  6. In guessno, you can flatten the if test to a single level. First check if the guess is correct, then check if no1 is right, then check if no2 is right, then check if the numbers are consecutive, then the guess is high, then check if the guess is low.
  7. The loop should terminate, or better yet raise an exception, if there is a nonsensical result. It currently just keeps running.
  8. The while test should probably just run forever, and break out of it when it is done.
  9. You should only run main() if __name__ == '__main__'. In other words, only run main() automatically if the file is run as a script.
\$\endgroup\$
0

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.