I know this code works, but is it correct to define them as functions or should I use something else?

import math

def Fibonnacci_sequence():
    i=int(input("How many numbers of the fibonnacci sequence do you want to see: "))
    a = 1
    b = 1
    n = 1
    n = n + 1 
    while n < i:
        c = a + b
        n = n + 1
        a = b
        b = c

def Fib_seqnum():
    i=int(input("What number of the fibonnacci sequence do you want to know: "))
    a = 1
    b = 1
    n = 1
    n = n + 1 
    while n < i:
        c = a + b
        n = n + 1
        a = b
        b = c

def solve_qaudratic():
    a=float(input("Enter A: "))
    b=float(input("Enter B: "))
    c=float(input("Enter C: "))
    d = (b*b) - 4*a*c
    if d < 0:
        print("No real roots")
    elif d == 0:
        x = -b/(2*a)
        print("equal roots" (x))
        x = math.sqrt(d)
        i = (-b -x)/(2*a)
        o = (-b +x)/(2*a)
        print("X1 = " (i))
        print("X2 = " (o))

print("Welcome to maths")
print("PRESS 9 TO EXIT")
answer=int(input("Enter Choice: " ))
if answer==1:
elif answer==2:
elif answer==3:
elif answer==9:

closed as off-topic by 200_success Dec 13 '15 at 21:34

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Your print() calls in solve_qaudratic() aren't syntactically valid. Please ensure that the code works as posted. \$\endgroup\$ – 200_success Dec 13 '15 at 21:36
  • \$\begingroup\$ it does work i have tested it \$\endgroup\$ – Steve Dec 14 '15 at 7:05
  • \$\begingroup\$ It doesn't work. I have tested it. TypeError: 'str' object is not callable \$\endgroup\$ – 200_success Dec 14 '15 at 7:07
  • \$\begingroup\$ copy it i just did and it worked perfectly \$\endgroup\$ – Steve Dec 14 '15 at 7:21
  • \$\begingroup\$ That's exactly what I did. Fails as reported in both Python 2 and Python 3. \$\endgroup\$ – 200_success Dec 14 '15 at 7:21

Your functions do too many things:

  • get input from user
  • do some calculation
  • print result

It would be better to separate these responsibilities, following the single responsibility principle.

When you separate these responsibilities, the Fibonacci generation logic will naturally end up in its own function, and the reusability becomes obvious.

Consider this rewrite of your Fibonnacci_sequence function:

def fib_sequence(n):
    >>> fib_sequence(10)
    [1, 1, 2, 3, 5, 8, 13, 21, 34, 55]

    :param n: the number of items to calculate
    :return: list of the first n items in the sequence
    a = 1
    b = 1
    k = 2
    seq = [a, b]
    while k < n:
        c = a + b
        k += 1
        a = b
        b = c

    return seq

The improvements:

  • removed the printing
  • removed the user input
  • follows PEP8
  • better variable names
  • documented
  • has a doctest

The most interesting part is the doctest. You can run doctests with the command:

python -m doctest script.py

Let's add a bunch more doctests:

>>> fib_sequence(0)

>>> fib_sequence(1)

>>> fib_sequence(2)
[1, 1]

>>> fib_sequence(3)
[1, 1, 2]

And let's rerun the doctests. Which of these new tests will fail? fib_sequence(0) and fib_sequence(1) will fail. Just like your original implementation always printed the first two 1s, this one also always returns a list starting with [1, 1, ...]. The good thing is that now that we have the tests, we can go ahead and confidently refactor the implementation.

As a matter of fact, a nice way to generate sequences in Python is using generators. Here's an implementation of the Fibonacci sequence using a generator:

def fib():
    yield 1
    yield 1
    prev = 1
    current = 1
    while True:
        prev, current = current, prev + current
        yield current

Now we can rewrite fib_sequence in terms of this new fib function:

it = fib()
return [next(it) for _ in range(n)]

If we rerun the doctests, now they all pass. We can further simplify this implementation using itertools.islice:

return list(itertools.islice(fib(), 0, n, 1))

To finish up, let's rewrite Fibonnacci_sequence in terms of fib_sequence:

def fibonacci_sequence():
    n = int(input("How many numbers of the Fibonacci sequence do you want to see: "))
    for num in fib_sequence(n):

It's a lot cleaner isn't it.

I invite you to write a fib_nth method similarly, following the same logical steps we took for fib_sequence:

  • remove the prompting and printing, make it return a value
  • write doctests, for all the interesting cases
  • rewrite fib_nth in terms of fib
  • rewrite fib_seqnum in terms of fib_nth

Not the answer you're looking for? Browse other questions tagged or ask your own question.