4
\$\begingroup\$

I am learning Python, and this is one of the program assignments I have to do.

I need to write code to prompt user to enter Fibonacci numbers continuously until it's greater than 50. If everything correct, screen show 'Well done', otherwise 'Try again'.

While my code is working correctly, I would like to know how I should write it better.

My code:

# Init variables
t = 0
prev_2 = 1
prev_1 = 0
run = 1

# Run while loop to prompt user enter Fibonacci number
while (run):
   text = input('Enter the next Fibonacci number >')
   if (text.isdigit()):
      t = int(text)
      if t == prev_2 + prev_1:
         if t <= 50:
            prev_2 = prev_1
            prev_1 = t
         else:
            print('Well done')
            run = 0
      else:
         print('Try again')
         run = 0
   else:
      print('Try again')
      run = 0
\$\endgroup\$
1
  • \$\begingroup\$ Great job. I think your code would benefit from naming variables more clearly, to a point where it should be possible to understand what it is without having to look at the code. t have already been mentioned in the answers. I would say that prev_1 is the last Fibonacci number, and prev_2 is the one before that, but according to the code it's the other way around. Naming them fibonacci_last and fibonacci_second_to_last, or prev_highest and prev_lowest or something similar makes it clearer how they are used. \$\endgroup\$ – Polygorial Apr 13 '20 at 11:47
5
\$\begingroup\$

First, run should be a Boolean value (True or False). 1 and 0 work, but they're far less clear. Note how much more sense this makes:

. . .
run = True

while (run):
   text = input('Enter the next Fibonacci number >')
   if (text.isdigit()):
      t = int(text)
      if t == prev_2 + prev_1:
         if t <= 50:
            prev_2 = prev_1
            prev_1 = t
         else:
            print('Well done')
            run = False
      else:
         print('Try again')
         run = False
   else:
      print('Try again')
      run = False

Instead of using a run flag though to exit the loop, I think it would be a lot cleaner to just break when you want to leave:

while (True):
   text = input('Enter the next Fibonacci number >')
   if (text.isdigit()):
      t = int(text)
      if t == prev_2 + prev_1:
         if t <= 50:
            prev_2 = prev_1
            prev_1 = t
         else:
            print('Well done')
            break
      else:
         print('Try again')
         break
   else:
      print('Try again')
      break

Instead of doing a isdigit check, you can just catch the ValueError that int raises:

while (True):
    text = input('Enter the next Fibonacci number >')
    try:
        t = int(text)
    except ValueError:
        print('Try again')
        break

    if t == prev_2 + prev_1:
        if t <= 50:
            prev_2 = prev_1
            prev_1 = t
        else:
            print('Well done')
            break
    else:
        print('Try again')
        break

This goes with Python's "It's better to ask for forgiveness than permission" philosophy.


Some other things:

  • Don't put parenthesis around the condition of while and if statements unless you really feel that they help readability in a particular case.

  • You don't need to have t assigned outside of the loop. I'd also give t a better name like user_input:

    prev_2 = 1
    prev_1 = 0
    
    # Run while loop to prompt user enter Fibonacci number
    while True:
        text = input('Enter the next Fibonacci number >')
        try:
            user_input = int(text)
        except ValueError:
            print('Try again')
            break
    
        if user_input == prev_2 + prev_1:
            if user_input <= 50:
                prev_2 = prev_1
                prev_1 = user_input
            else:
                print('Well done')
                break
        else:
            print('Try again')
            break
    
\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for your help. \$\endgroup\$ – ThangNguyen Apr 13 '20 at 4:49
6
\$\begingroup\$

A few minor tips on making the code more compact:

  1. Remove unneeded variables/initializations. In particular your t variable doesn't need to be initialized outside the loop since its value is always set within the loop before it's used.

  2. The run variable can also be eliminated; make the loop while True and use a break to escape it when you're done.

  3. Instead of doing text.isdigit() just do the int conversion inside a try block.

  4. Combine your two "Try again" cases into a single block of code rather than repeating yourself.

With those changes the code looks like:

# Init previous two values of Fibonacci sequence
prev_2 = 1
prev_1 = 0

# Run while loop to prompt user enter Fibonacci number
while True:
    try:
        t = int(input('Enter the next Fibonacci number >'))
        if t != prev_2 + prev_1:
            raise ValueError('not the next Fibonacci number!')
        if t > 50:
            print('Well done')
            break
        # move to next value in the sequence
        prev_2, prev_1 = prev_1, t
    except ValueError:
        print('Try again')
        break

A larger tip on improving the code would be to separate the two distinct tasks the code is doing: computing the Fibonacci sequence up to 50, and quizzing the user on it. Here's how I might write that:

# Init Fibonacci sequence up to first value past 50 (shfifty-five)
fib = [0, 1]
while fib[-1] <= 50:
    fib.append(fib[-2] + fib[-1])

# Prompt the user for all those values (after the initial 0, 1)
for n in fib[2:]:
    try:
        if int(input('Enter the next Fibonacci number >')) != n:
            raise ValueError('not the next Fibonacci number!')
    except ValueError:
        print('Try again')
        break
else:
    print('Well done')

Now the code is in two smaller parts that are each a lot simpler than the previous version -- the first few lines compute the Fibonacci sequence in a very straightforward way, and the rest of the code simply iterates over the sequence and checks the input against it.

In a larger project you'd probably take these two "chunks" of code and define them as functions (you'd have one function that generates the sequence and another that handles the quizzing), but even within a function it's valuable to think about how you can break a problem does into distinct simple parts (and once you've done that, the next step of refactoring it into smaller functions becomes easy).

\$\endgroup\$
1
  • \$\begingroup\$ Thank you very much for your help. \$\endgroup\$ – ThangNguyen Apr 13 '20 at 4:50
6
\$\begingroup\$

The 3 answers already given are great. I'd just like to point out a minor bug with the original code not mentioned in these 3 answers.

The str.isdigit() function can yield false positives. If the user enters 1²₃, the isdigit() method will return True because all the characters are Unicode digits, but int(...) will raise an exception because the characters are not all decimal digits.

The function you wanted was str.isdecimal().

>>> text = "1²₃"
>>> text.isdigit()
True
>>> int(text)
Traceback (most recent call last):
  module __main__ line 141
    traceback.print_exc()
  module <module> line 1
    int(text)
ValueError: invalid literal for int() with base 10: '1²₃'
>>> text.isdecimal()
False
>>> 
\$\endgroup\$
4
  • \$\begingroup\$ Thank you very much for your help. \$\endgroup\$ – ThangNguyen Apr 13 '20 at 4:52
  • 1
    \$\begingroup\$ "Easier to ask for forgiveness than permission" is a common Python coding style. In this instance you would write try: int(text) except ValueError: .... It could have prevented this bug because it doesn't require the writer to know the circumstances under which int() will fail. \$\endgroup\$ – kangalioo Apr 13 '20 at 10:26
  • 1
    \$\begingroup\$ @kangalioo Two of the other three answers had already more than adequately covered the try: int(text) except ValueError: "easier to ask forgiveness than permission" approach. I was just pointing out an actual bug in the OP's "ask permission" code that had not been noticed or mentioned. At some point, the OP may need the .isdecimal() function for some other code, and may remember this answer and not erroneously use .isdigit(). Or they might actually need the broader .isdigit() functionality and remember it already exists. Know how to use all the tools in the toolbox. \$\endgroup\$ – AJNeufeld Apr 13 '20 at 17:06
  • \$\begingroup\$ True, sorry. I suppose I should have read the other answers before making that comment \$\endgroup\$ – kangalioo Apr 13 '20 at 20:43
4
\$\begingroup\$

Welcome to CodeReview! This is a great first question.

Breaking out of a loop

You don't necessarily need to have a condition at the declaration of the loop, like while (run):. Instead,

# Run while loop to prompt user enter Fibonacci number
while True:
   text = input('Enter the next Fibonacci number >')
   if text.isdigit():
      t = int(text)
      if t == prev_2 + prev_1:
         if t <= 50:
            prev_2 = prev_1
            prev_1 = t
         else:
            print('Well done')
            break
      else:
         print('Try again')
         break
   else:
      print('Try again')
      break

Two other answers just popped up so I will leave this here.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for your help. \$\endgroup\$ – ThangNguyen Apr 13 '20 at 4:51
  • 1
    \$\begingroup\$ This has two problems. prev_2 and prev_1 are not inititalized. Two of the break statements should be continue instead (only goal reached should end the program). At this moment, all answers recommending while True: seem to have the 'break instead of continue' problem, \$\endgroup\$ – Bit Chaser Apr 13 '20 at 19:00
  • 1
    \$\begingroup\$ It may be that the code doesn't do the right thing, but the right thing wasn't the direct intent; the intent was to replicate what the OP was doing. \$\endgroup\$ – Reinderien Apr 13 '20 at 20:24
0
\$\begingroup\$

Others have given many great suggestions on the code style, but I'd like to point out that your code's behavior does not exactly match your description of what it should do. You said:

I need to write code to prompt user to enter Fibonacci numbers continuously until it's greater than 50. If everything correct, screen show 'Well done', otherwise 'Try again'.

Based on this description, it sounds like the code should be prompting for numbers continuously, even if they give you a wrong answer, until they give you a number greater than 50. Only then would you reveal whether they had gotten it correct or not.

I didn't see anyone else point this out, and the hardest part of programming is understanding exactly what your program should do. I may be understanding the description wrong, of course, but I would double-check that your program meets the problem specification.

\$\endgroup\$
1
  • \$\begingroup\$ Hi, I mean the software prompt the user input the Fibonacci number in the series. If the input number is correct, software will prompt user to enter the next one. Until it greater than 50 and print 'Well done'. If input number is wrong, software will print 'Try again' and quit. \$\endgroup\$ – ThangNguyen Apr 15 '20 at 17:22

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.