7
\$\begingroup\$

Here's the code I'm having trouble shortening. I am currently a beginner in Python 3.x.

from os import system

def fibonacci(terms):
    count = 0
    x = 0
    y = 1
    z = x + y
    if terms <= 0:
        print("Enter a positive integer.")
    elif terms == 1:
        print("Fibonacci sequence in " + str(terms) + " terms.")
        print(x)
    else:
        print("Printing " + str(terms) + " terms.")
        while count < terms:
            print(x)
            z = x + y
            x = y
            y = z
            count += 1

while True:
    user_input = int(input("How many terms? "))
    fibonacci(user_input)
    loop = input("Again? (Y/n) ")
    if loop == "Y" or loop == "":
        system("cls")
    else:
        exit()

I would appreciate if there will be someone who will be able to make this as efficient as possible.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ Ahoy! 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.". Feel free to edit and give it a different title if there is something more appropriate \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 23 '20 at 4:48
15
\$\begingroup\$

Separate responsibilities

Your functions should, where possible, limit themselves to doing one thing only. This is the single-responsibility principle. In case of fibonacci(), I expect this function to just calculate the desired Fibonacci number, and not deal with user input and output. This shortens the function and makes it easier to reason about. So:

def fibonacci(terms):
    count = 0
    x = 0
    y = 1
    z = x + y

    while count < terms:
        yield x
        z = x + y
        x = y
        y = z
        count += 1

Instead of printing the values, it yields them. To print the values, use the following:

terms = int(input("How many terms? "))
for number in fibonacci(terms):
    print(number)

You can even go futher and make fibonacci() an infinite generator, and leave it up to the caller to decide when to stop:

import itertools

def fibonacci():
    x = 0
    y = 1

    while True:
        yield x
        z = x + y
        x = y
        y = z

terms = int(input("How many terms? "))
for number in itertools.islice(fibonacci(), terms):
    print(number)

Here I used itertools.islice() to take only the first terms items from the generator.

If you want to handle invalid input gracefully, I suggest you write another function whose responsibility is asking the user for a postivive integer:

def get_positive_integer(prompt):
    while True:
        user_input = int(input(prompt))
        if ...:            
            return user_input
        print("Enter a positive integer.")

Avoid using os.system()

Using os.system() to clear the screen is very inefficient and not portable. Of course it is prettier to clear the screen, but if it is not really important for your program, I would just not do it; portability is more important.

\$\endgroup\$
4
  • 1
    \$\begingroup\$ as an addition OP constructs: print("Fibonacci sequence in " + str(terms) + " terms." could use format instead. \$\endgroup\$ – Jean-François Fabre Nov 22 '20 at 16:45
  • 5
    \$\begingroup\$ @Jean-FrançoisFabre Format is old, f-strings are good \$\endgroup\$ – Aryan Parekh Nov 22 '20 at 16:45
  • 1
    \$\begingroup\$ yeah, unless you're stuck with a legacy version of python. \$\endgroup\$ – Jean-François Fabre Nov 22 '20 at 16:46
  • \$\begingroup\$ One drawback of f-strings is that they don't really work well in combination with internationalization. \$\endgroup\$ – G. Sliepen Nov 22 '20 at 16:48
7
\$\begingroup\$
        z = x + y
        x = y
        y = z

is not idiomatic Python. Consider

        x, y = y, x + y

Regarding optimization, since you want all terms, there is not much to optimize. If wanted a specific term, a matrix exponentiation would be a way to go.

\$\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.