Everyone loves Fibonacci

I was bored and burnt out on more serious projects.... until I saw this Computerphile video on YouTube. Now I'm inspired again. Now I feel like spending some time implementing that recursive algorithm in a little console program. Fortunately, recursion is unbearably slow in this case. So, I ended up writing a small class that implements a loop algorithm to find any \$F_n\$.

I'm just learning C#, so I'm most concerned with best practices and style. I want to get a good foundation though, so all feedback is welcome.

Particular Concerns

• Is my indentation correct?

• Have I named my variables horribly? (I think they smell a bit.)

• Which implementation is better? I'm feeling like it's six of one or a half doz. of the other.

• Should I be concerned about hitting the upper boundary of the ulong type? Fibonacci numbers grow very quickly. Is there a better representation of very large numbers?

Fibonacci

class Fibonacci
{
    private int _n;
    private ulong _value;
    private ulong _FnMinus1;
    private ulong _FnMinus2;

    public int n 
    {
        get { return _n; }
        set { _n = value; } 
    }

    public ulong Value 
    { 
        get {return _value;}
        //set { _value = value; }
    }

    public void Calculate() 
    { 
        if (n <= 0)
        {
            throw new ArgumentOutOfRangeException("n","Can't calculate Fn when n is less than or equal to zero.");
        }
        else if (n == 1 || n == 2)
        {
            _value = 1;
        }
        else
        {
            //initialize previous results
            _FnMinus1 = 1;
            _FnMinus2 = 1;

            for (int i = 0; i <= (n - 2) ; i++)
            {
                _value = _FnMinus1 + _FnMinus2;
                _FnMinus2 = _FnMinus1;
                _FnMinus1 = _value;
            }
        }
    }

    //constructors
    public Fibonacci(int n)
    {
        _n = n;
        Calculate();
    }

    public Fibonacci() { }

}

Implementation1

```
class

Indentation and formatting are fine, as is the algorithm itself.

Your naming is inconsistent, compare n and Value; _value and _FnMinus1. There are a variety of conventions to follow, but consistency is the most important thing.

_FnMinus1 and _FnMinus2 are only used in one method, so they can be made local to that method.

The API is a little odd:

fib.n = i;
fib.Calculate();
int result = fib.Value;

It would be more convenient if I could do this with one line, like int result = fib.Calculate(i), which would return the value. Then it seems that n and Value are unnecessary.

Furthermore, right now there is nothing that keeps n and Value synchronized. The class should maintain the invariant that Value is always the nth Fibanacci number. You could do the work in the n assignment or mark Value as dirty when n changes. Either way you essentially get rid of Calculate, so that's another approach. I prefer getting rid of n and Value, I just mention this in case you think it makes sense to keep them.

Between Implementation 1 and Implementation 2, it's pretty much a wash. 1 has the benefit of only making one allocation. Since the class is (going to be) stateless, it's perfectly reusable. On the other hand 2 declares fib inside the loop, which minimizes its scope (which is good).

StackExchange Code Review Q#54769, answer score: 28