What is wrong with this Fibonacci function?

Question

Stumbled upon this example of bad C++ code in a blog post, without any explanation as to why it is considered "bad". I have my own ideas, but would like to hear experienced C++ devs on this.

unsigned int Fibonacci (unsigned int n)
{
    if (n == 0 || n == 1)
        return n;
    else
        return Fibonacci (n - 1U) + Fibonacci (n - 2U);
}

You should remove "C++" from this question. This algorithm really transcends languages, it's bad with _any_ implementation! — Tom, Jul 07 '11 at 04:26
@Tom, true, but there are likely some functional languages which would probably optimize this just fine. I recall a few do automatic memoization. — edA-qa mort-ora-y, Jul 07 '11 at 05:44

score 7 · Accepted Answer · answered Jul 07 '11 at 00:55

7

Perhaps because it runs in exponential time?

answered Jul 07 '11 at 00:55

Aaron Klotz

11,287
1
28
22

2

And it's far too many lines of code for this particular method: `return (n < 2) ? n : Fibonacci(n-1U) + Fibonacci(n-2U);` On another note, the main issue with the closed form equation is that it'll probably lead to floating point errors. – Steve Wang Jul 07 '11 at 01:09
More importantly it consumes exponential CPU stack memory (because of exponential depth of recursion). Its bad because it runs only for small input numbers. For larger numbers it crashes because of stack overflow. – Serge Dundich Jul 07 '11 at 08:55
Shouldn't be exponential stack memory, since C++ does not automatically parallelize. So you won't get more than O(n) stack size. – Steve Wang Jul 07 '11 at 17:14
@Steve Wang: `O(n) = O( exp(C*sizeof(n)) )` **is** exponential stack size. Although good point since I just realized that running time is `O(2^n)=O(exp(C*n))=O(exp(C*exp(C1*sizeof(n)))` that actually severely super-exponential. Just horrible. :) – Serge Dundich Jul 08 '11 at 06:08
If you calculate it based on the number of bits in n then it's exponential stack size.. but I think everybody calls this O(n). – Karoly Horvath Jul 12 '11 at 20:40

score 7 · Answer 2 · answered Jul 07 '11 at 01:03

7

To elaborate on the above statement: since you're not memoizing, you spawn 2 processes with the first call, and each of those spawns two processes, and so forth down the chain until you hit the base case.

Three ways to avoid this: 1) Memoize, 2) Do it iteratively, or 3) Use the closed form equation for the Fibonacci sequence. :D

answered Jul 07 '11 at 01:03

Steve Wang

1,814
1
16
12

1

+1 Though they're not *processes* in the literal sense, of course. – Jon Purdy Jul 07 '11 at 01:08
Well, yes. They're more of processes in the sense that a "recursive process" is a process. – Steve Wang Jul 07 '11 at 01:10
1

Whoah. Thanks for using the term "memoize". I learned a new word today. :) – Chris Frederick Jul 07 '11 at 01:24

score 4 · Answer 3 · answered Jul 07 '11 at 01:39

Most values of Fibonnacci (n) are calculated twice.

For example, Fibonacci(5) calls Fibonacci(4) and Fibonacci(3).

That Fibonacci(4) in turn calls Fibonacci(3) and Fibonacci(2).

See how Fibonacci(3) in this example is called twice? That's where the memoize would help, but the algorithm, while interesting and recursive, is not efficient. It would be preferable to use a more efficient algorithm than to memoize an inefficient one.

Serge Dundich · Answer 4 · 2011-07-08T06:11:54.583

Exponential running time (and may be even super-exponential - like in this case) is bearable if you have like eternity to wait until program finishes.

But nothing in the world can possibly handle exponential memory consumption - especially exponential program stack consumption (due to exponential depth of recursion). This program will just crash because of stack overflow with big enough input number.

It is not like "recursion is evil". Recursion is acceptable if the depth of recursion is limited by some small value (e.g. if it is logarithmic of the input size or not more than sizeof(int) or something). But not when proportional to the input value n.

score 1 · Answer 5 · answered Jul 07 '11 at 01:12

Some people will say it's bad because it uses recursion or because it uses it without memoization, which is quite reasonable since there are approaches that only use iteration and save values that would be repeated in auxiliary variables, other will point to the fact that it can be calculated using Binet's Formula, to a certain degree of precision.

Other people will say it's the multiple return points, and even more strangely someone might say it's bad because the else is superfluous and it could be removed to save one line.

What is wrong with this Fibonacci function?

5 Answers5