Fibonacci generator python implementation

Question

I wrote this code for a fibonacci sequence generator. Can anyone tell me whether my code is designed well? if not, why?

import logging

def fibonacci_calc(current_val, current_pos=0, max_seq_length=5, sequence_list=[]):
    try:
        if(current_pos != 0 and len(sequence_list) == 0):
            raise Exception("Please initalize {} argument: {} with 0".format(fibonacci_calc.__name__, fibonacci_calc.__code__.co_varnames[1]))
        else:
            if(len(sequence_list) == 0):
                print("Initialization, added 0 and starting number {} to sequence list".format(current_val))
                sequence_list.append(0)
                sequence_list.append(current_val)
                current_pos = 1
                print(sequence_list)
                fibonacci_calc(current_val, current_pos, max_seq_length, sequence_list)
            elif(len(sequence_list) == max_seq_length):
                print('{} values in the sequence'.format(max_seq_length))
                return 
            else:
                print("Current position: {}, Total values: {}  calculating next value".format(current_pos, current_pos+1))
                next_val = sequence_list[current_pos] + sequence_list[current_pos-1]
                sequence_list.append(next_val)
                print("Current sequence list {}".format(sequence_list))
                fibonacci_calc(next_val, current_pos+1, max_seq_length, sequence_list)
    except Exception:
        logging.exception('ERROR')

fibonacci_calc(19,0, 7)

What is it supposed to do? At the very very least, show the result of the fibonacci_calc(19,0, 7) call you already do. — Manuel
– Manuel, Commented Apr 14, 2021 at 12:00

Airistotal · Accepted Answer · 2021-04-14 13:19:31Z

2

tldr; I'd say that this function is too complicated.

Recursion should be avoided when possible as an iterative method is usually more readable and could be faster. Here's some other things to consider when making a recursive function: https://gist.github.com/abinoda/5593052#when-to-use-recursion-vs-iteration.

I think that some of the naming is not as clear as it could be, although that could just be an effect of the function being recursive. Consider changing parameter max_seq_length to seq_length. I couldn't quite see any scenario where you would give less results than the value passed in to max_seq_length.

Another item that could use some thought is your use of raising an exception when validating your input:

if(current_pos != 0 and len(sequence_list) == 0):
    raise Exception("Please initalize {} argument: {} with 0".format(fibonacci_calc.__name__, fibonacci_calc.__code__.co_varnames[1]))

Here, you would likely be better off just logging a message instead of introducing more overhead by using an exception.

Here's an example of a function that addresses my concerns:

def get_fibonacci_sequence(start_num, seq_length):
    if (seq_length < 1):
        return []
    elif (seq_length == 1):
        return [start_num]
    elif (seq_length == 2):
        return [start_num, start_num]

    fibonacci_seq = [start_num, start_num]

    for i in range(2, seq_length):
        fibonacci_seq.append(fibonacci_seq[i - 1] + fibonacci_seq[i - 2])

    return fibonacci_seq

print(get_fibonacci_sequence(19, 7))

PS. I'm assuming all the print statements are just debugging aids and wouldn't be included in the final version. Those messages are likely superfluous for other purposes.

edited Apr 14, 2021 at 13:19

answered Apr 14, 2021 at 2:49

Airistotal

1365 bronze badges

\$\begingroup\$ Interesting implementation thank you, are there any advantages to uses multiple if's instead of if-elif-else statements apart from less characters? \$\endgroup\$

Malemna
– Malemna

2021-04-14 03:22:51 +00:00
Commented Apr 14, 2021 at 3:22
1

\$\begingroup\$ @Malemna its not just multiple ifs; it's the early returns \$\endgroup\$

hjpotter92
– hjpotter92

2021-04-14 06:44:58 +00:00
Commented Apr 14, 2021 at 6:44
\$\begingroup\$ No, no advantages to multiple ifs, that was just an oversight on my part, sorry! Fixed it. @hjpotter92 The early returns are shortcircuits for cases where we automatically know the answer. I agree, multiple return statements hidden throughout a large function is bad, but they are usually fine if they are at the beginning or at the end of a function. \$\endgroup\$

Airistotal
– Airistotal

2021-04-14 13:24:36 +00:00
Commented Apr 14, 2021 at 13:24
\$\begingroup\$ Hey thanks dude, learned alot! Do you have any recommendations on any books that covers alot of these bad practices? \$\endgroup\$

Malemna
– Malemna

2021-04-14 13:30:03 +00:00
Commented Apr 14, 2021 at 13:30
1

\$\begingroup\$ I would argue using elif here is not necessary better than multiple if statements, since only one can be true and the subsequent ifs will not be evaluated anyways, if one of the conditions is met. I personally find multiple ifs clearer as they mark different, mutually exclusive exit conditions, but that's just personal preference. =) \$\endgroup\$

riskypenguin
– riskypenguin

2021-04-14 16:17:44 +00:00
Commented Apr 14, 2021 at 16:17

| Show 2 more comments

riskypenguin · Accepted Answer · 2021-04-14 11:59:23Z

Python Mutable Defaults Are The Source of All Evil

Using a mutable type as a default argument is generally not a good idea. You are using an empty list [] as the default value sequence_list in your function header:

def fibonacci_calc(current_val, current_pos=0, max_seq_length=5, sequence_list=[]):
    ...

I have made this mistake before and the bugs it produced were incredibly hard to fix before I knew what I was looking for. You should only ever do that if you have a good reason for it and know exactly what you're doing. I personally haven't encountered such use cases, but there might be some.

The problems will only become appearant once you call the function multiple times during one execution. Go ahead and try this out for yourself, here is one example:

fibonacci_calc(10, 0, 3)

produces

Initialization, added 0 and starting number 10 to sequence list
[0, 10]
Current position: 1, Total values: 2  calculating next value
Current sequence list [0, 10, 10]
3 values in the sequence

while

fibonacci_calc(1, 0, 2)
print()
fibonacci_calc(10, 0, 3)

produces

Initialization, added 0 and starting number 1 to sequence list
[0, 1]
2 values in the sequence

Current position: 0, Total values: 1  calculating next value
Current sequence list [0, 1, 1]
3 values in the sequence

What should you do instead?

The recommended way is to switch from

def this_can_go_wrong_easily(my_list=[]):
    my_list.append(10)
    return my_list

to

def this_behaves_as_expected(my_list=None):
    if my_list is None:
        my_list = []

    my_list.append(10)
    return my_list

This will assign a new empty list [] as a default value to sequence_list every time the function is called, instead of once at compile time.

Here's an overview of mutable and immutable types in Python:

Mutable objects:
list, dict, set, bytearray

Immutable objects:
int, float, str, tuple, frozenset, bytes, complex

Interesting thank you, im learning so much about the difference between good and bad code — Malemna
– Malemna, Commented Apr 14, 2021 at 13:06

Marc · Accepted Answer · 2021-04-14 10:27:38Z

@Airistotal already provided a nice review, I will focus on the generator part.

I wrote this code for a fibonacci sequence generator

In Python, a generator is a function that behaves like an iterator. A generator yields items instead of returning a list.

An example of Fibonacci generator:

def fibonacci_generator(b=1):
    a = 0
    while True:
        yield a
        a, b = b, a + b

for num in fibonacci_generator():
    print(num, end=" ")

It prints:

0 1 1 2 3 5 8 13 21 34 55 89 144 ....

To return selected elements of a generator, you can use itertools.islice. For example, printing the first 10 numbers:

from itertools import islice

first_10_numbers = list(islice(fibonacci_generator(), 10))
print(first_10_numbers)

# output: [0, 1, 1, 2, 3, 5, 8, 13, 21, 34]

In your case, I see that current_val is the starting point of the sequence. That would be the argument b of the function fibonacci_generator:

print(list(islice(fibonacci_generator(19), 8)))
# output: [0, 19, 19, 38, 57, 95, 152, 247]

Which is the same output as your function:

print(fibonacci_calc(19, 0, 8))
# output: [0, 19, 19, 38, 57, 95, 152, 247]

Interesting thank you, im learning so much about the difference between good and bad code — Malemna
– Malemna, Commented Apr 14, 2021 at 13:06

Stack Exchange Network

Fibonacci generator python implementation

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Fibonacci generator python implementation

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions