Does everyone still love Fibonacci?

I'm following up on this question. I made a few decisions about my model and implemented recommendations accordingly.

TL;DR the link


Uses a loop algorithm to return a Fibonacci number \$Fn\$ of any given ordinal position \$n\$.

Changes

• I'm now using a BigInteger type to avoid Integer Overflow. I can now return an arbitrarily large Fibonacci number.

• Clarified private variable names.

• Calculate is now an internal method. Setting the OrdinalPosition recalculates the value automatically, so the client code is no longer responsible for syncing the object.

• I added a more complete console implementation.

Questions

• Would this class benefit at all by actually inheriting from BigInteger?

• Are the names better? They really stunk in my last version.

• Tear it apart. I can take it. No beginner tag on this one.

Fibonacci

```
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Numerics;

namespace Challenges
{
class Fibonacci
{

private int ordinalPosition;
public int OrdinalPosition
{
get { return ordinalPosition; }
set
{
ordinalPosition = value;
Calculate();
}
}
public BigInteger Value { get; private set; }

private void Calculate()
{
if (this.OrdinalPosition <= 0)
{
throw new ArgumentOutOfRangeException("OrdinalPosition","Can't calculate Fn when n is less than or equal to zero.");
}
else if (this.OrdinalPosition == 1 || this.OrdinalPosition == 2)
{
this.Value = 1;
}
else
{
//initialize previous results
BigInteger previous1 = 1;
BigInteger previous2 = 1;

for (int i = 0; i < (this.OrdinalPosition - 2); i ++ )
{

Class vs Method

This class is structured so that you give it an input (OrdinalPosition), and then from it you get an output (Value). Well, we already have something that's much more specifically suited for that than a class: a method! Instead of

private void Calculate()

you should have

public BigInteger Calculate(int ordinalPosition)

There are several reasons that this is preferable:

• Right now, you can hit problems at runtime by trying to get Value without first setting OrdinalPosition. By having the value be returned from a method, there's no way to run into this problem as long as your code compiles.

• Following on from the above, classes that have to have their properties accessed- or methods used- in a certain order are usually something to be avoided. They just add potential sources of error. They also add extra unwanted sources of coupling. If class A hands an instance of class B to an instance of class C, C should be able to trust that it can use B as it wants. If it has to know about whether or not B has had certain methods called on it, then it has to know about how A works, so this couples C to both A and B.

• In theory, properties are simply syntactic sugar, with a get method, a set method, and a backing field which is not publicly accessible. However, because they look like fields, conceptually we often think of them as like fields. One manifestation of this is that we generally don't want them to do a lot of work beyond the getting and setting of the variable. Having Calculate called every time the ordinal position is set violates this.

• Class-level variables aren't usually something to worry about, but when they should be at the method-level, putting them at the class-level can cause trouble. It can couple methods too tightly to one another (in that they share their variables, rather than strictly passing each other what they need as and when they need it), and it can be a potential source of bugs. For example, I can easily imagine in a moment of thoughtlessness you might have Calculate set OrdinalPosition for some reason, which would lead to an infinite loop.

• This coupling of methods also leads to less flexibility. If one method changes in a way that involves changing the class-level field, then all the other methods which access it have to worry about it too.

• It's generally less clear what the flow of this class's usage is. You can create it, then either immediately set the OrdinalPosition or wait until later. Why? Are there situations in which both of those are needed? And once the OrdinalPosition is set, I could either reset it to calculate for a different position, or create a new instance of the class with the new ordinal position. Why? Which am I supposed to do? If I just want the Value, why would I hold a reference to the whole class rather than just storing the value? If any additions are made to this class, I'll constantly have to be considering all these different usage scenarios. Likewise if I'm handed an instance of the class and know some other code holds the same instance, I'll have to keep questioning all the assumptions I can make about how that code uses it.

Okay, hopefully that's convincing. So the change is simple, as I suggested, just delete the OrdinalPosition, ordinalPosition and Value fields, and the constructor overload which sets OrdinalPosition, and change the signature of Calculate.

Static?

You're now left with just two members: a parameterless constructor and the new Calculate method. In fact, an empty parameterless constructor isn't needed now that the other one is gone, so that can be removed too. And at this point you might think that perhaps you should make the method- or in fact the class- static. There is, after all, no preserved state, and nothing instance-specific that's required.

Well, this is often a judgement call. Here are a few things you can't do if it's static:

• Add state- for example some cached information. Well, you can, but you now have to be very careful. Will it always be relevant globally? Will I always be happy for code anywhere in my project to be able to call methods which potentially modify this state? If you might want to maintain two versions of this at once- well, that's what instance members are for

• Swap out implementations. Will you ever want to use different implementations of this class? Probably not, but it's an important question you'd need to be sure of before making it static. If you'd ever want to have IFibonacci, with different implementations of your Calculate method, then that rules out making it static.

• Mock out the class for testing. This is an example of the above, but it's a special enough one that it deserves its own bullet point. If you use this Fibonacci class inside another class, and you want to test that other class, you may want to stop it from actually using a real calculation of Fibonacci. That's parti

StackExchange Code Review Q#54894, answer score: 14