HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Two Fibonacci functions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
functionstwofibonacci

Problem

These are two of tasks, which I had to do for the recruitment process to be started in some company. Unfortunately they didn't like it so I've decided to share it here for discussion.

-
Add up all the even elements of the Fibonacci sequence below 1000. In my solution it is a method fibSumEven.

-
Write a recursive function that calculates the Fibonacci sequence up to 1000. In my solution it is a method fibR.

Fibonacci.h

#ifndef FIBONACCI_H_
#define FIBONACCI_H_

#include 
#include 
#include 

class CFibonacci
{

private:

    // Recursive function that calculates the Fibonacci sequence up to given LIMIT
    // limit - The limit, up to which sequence numbers will be calculated
    static int fibHelper(int limit, int x1, int x2)
    {
        int x = x1 + x2;
        if(x <= limit)
        {
            return fibHelper(limit, x2, x);
        }
        return x2;
    }

public:

    // A caller to recursive Fibonacci sequence up to given limit
    static int fibR(int limit = 1000)
    {
        if(limit <= 0)
        {
            return 0;
        }
        return fibHelper(limit, 0, 1);
    }

    //Adds up all the even elements of the Fibonacci sequence below limit
    static int fibSumEven(int limit=1000)
    {
        int x1 = 0, x2 = 1, x = 0, sum = 0;
        while(x <= limit)
        {
            x = x1 + x2;
            if(x % 2 == 0)
            {
                sum += x;
            }
            x1 = x2;
            x2 = x;
        }
        return sum;
    }
};

#endif /* FIBONACCI_H_ */


There was a disclaimer:


Considering the ith element of the Fibonacci sequence Fib(i) = x, the element x is a natural number and is itself bounded by the value 1000 and not all the elements to Fib(1000).

Why could the solution have been disliked by the recruiters?

Solution

I don't like how you have all your logic in the header file. You should declare your methods and fields in the header and use the .cpp file for logic.

On the other hand, C++ is not strictly object oriented, the way C# and Java are. You didn't need a class for this solution, you could just have called fibHelper() from main().

Third point is your naming. fibHelper() actually isn't a helper function, it is the function that actually calculates the Fibonacci sequence. fibR() actually isn't recursive, like your comment says, it calls the recursive function. int x1 doesn't say what it actually does, although it is easy to guess because the program is only a couple lines long - in a large project, this could (and probably would) cause many weeks of time wasted catching bugs.

Fourth is consistency:

static int fibR(int limit = 1000)
static int fibSumEven(int limit=1000)


For the most part, you are consistent, and I like how you use spaces around your operators and braces on your one-line ifs.

Code Snippets

static int fibR(int limit = 1000)
static int fibSumEven(int limit=1000)

Context

StackExchange Code Review Q#94939, answer score: 7

Revisions (0)

No revisions yet.