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) 

Answer 1

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.

Answer 2

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 

Answer 3

@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]