0
\$\begingroup\$

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
    print(a)
    print(b)
    n = n + 1 
    while n < i:
        c = a + b
        print(c)
        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
    print(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))
    else:
        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 1 TO LOOK AT THE FIBONNACCI SEQUENCE")
print("PRESS 2 TO SOLVE A QUADRATIC")
print("PRESS 9 TO EXIT")
answer=int(input("Enter Choice: " ))
if answer==1:
    Fibonnacci_sequence()
elif answer==2:
    solve_qaudratic()
elif answer==3:
    Fib_seqnum()
elif answer==9:
    exit()
else:
    print("WHAT?!")
\$\endgroup\$
6
  • \$\begingroup\$ Your print() calls in solve_qaudratic() aren't syntactically valid. Please ensure that the code works as posted. \$\endgroup\$ Commented Dec 13, 2015 at 21:36
  • \$\begingroup\$ it does work i have tested it \$\endgroup\$
    – Steve
    Commented Dec 14, 2015 at 7:05
  • \$\begingroup\$ It doesn't work. I have tested it. TypeError: 'str' object is not callable \$\endgroup\$ Commented Dec 14, 2015 at 7:07
  • \$\begingroup\$ copy it i just did and it worked perfectly \$\endgroup\$
    – Steve
    Commented Dec 14, 2015 at 7:21
  • \$\begingroup\$ That's exactly what I did. Fails as reported in both Python 2 and Python 3. \$\endgroup\$ Commented Dec 14, 2015 at 7:21

1 Answer 1

4
\$\begingroup\$

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
        seq.append(c)
        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)
[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):
        print(num)

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

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