Does everyone still love Fibonacci?

Question

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

1. Would this class benefit at all by actually inheriting from BigInteger?
2. Are the names better? They really stunk in my last version.
3. 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 ++ )
                {
                    this.Value = previous1 + previous2;
                    previous2 = previous1;
                    previous1 = this.Value;
                }
            }
        }

        //constructors
        public Fibonacci(int n)
        {
            this.OrdinalPosition = n;
        }

        public Fibonacci(){}

    }
}

Console Program

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

namespace Challenges
{
    class Program
    {
         static void Main(string[] args)
        {
             Console.WriteLine("How many Fibbonacci numbers should I print?");

             int input;
             if (int.TryParse(Console.ReadLine(),out input))
             {
                 WriteOkMesage();
                 WriteLotsOfFibbonacci(input);   
             }
             else
             {
                 WriteNotAnIntegerMessage();
             }

            Console.WriteLine(Environment.NewLine + "Let's just pick one at random.");
            Console.WriteLine("What nth number Fibbonacci would you like?");

            if (int.TryParse(Console.ReadLine(),out input))
            {
                WriteOkMesage();
                WriteAFibbonacci(input);
            }
            else
            {
                WriteNotAnIntegerMessage();
            }

            Console.WriteLine("Press enter to close...");
            Console.ReadLine();
        }

        static private void WriteAFibbonacci(int n)
        {
            try
            {
                Fibonacci fib = new Fibonacci(n);
                Console.WriteLine("The answer is: " + fib.Value);
            }
            catch(ArgumentOutOfRangeException e)
            {
                WriteErrorMessage(e);
            }
        }

        static private void WriteLotsOfFibbonacci(int numberToPrint)
        {
            Fibonacci fib = new Fibonacci();
            try
            {
                for (int i = 1; i <= numberToPrint; i++)
                {
                    fib.OrdinalPosition = i;
                    Console.WriteLine(fib.Value);
                }
            }
            catch (ArgumentOutOfRangeException e)
            {
                WriteErrorMessage(e);
            }
        }

        static private void WriteOkMesage()
        {
            Console.WriteLine("Okay!" + Environment.NewLine);
        }

        static private void WriteErrorMessage(Exception e)
        {
            Console.WriteLine(e.Source + " " + e.Message);
        }

        static private void WriteNotAnIntegerMessage()
        {
            Console.WriteLine("That's not an integer! I can't process that.");
        }

    }
}

Answer 1

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 partially for speed, and partially so that you can tell the difference between bugs in that class, and bugs in your Fibonacci class. This again means creating a different implementation, so statics aren't appropriate.

These are judgement calls you need to make, but in this particular case, I'd suggest that the first point is persuasive and the second two are probably not. An example of state you might want to preserve is if you take a dynamic programming approach to Fibonacci, where you cache the results of previous calculations. This probably could be done with statics, but when dealing with static vs. instance, generally the best guideline is err on the side of instance. It costs very little and it's the safer option.

Unneeded else

A small style point, but one that can be important. If you exit a method- either by returning or throwing an exception- inside an if statement, there's no need to then have an else. By the time you've hit the else line, you already know the if condition wasn't true, or you'd have exited out of the method by then.

So you can cut out some unneeded lines and indentation, neatening up your Calculate method:

    private BigInteger Calculate(int ordinalPosition) 
    { 
        if (ordinalPosition <= 0)
        {
            throw new ArgumentOutOfRangeException("OrdinalPosition","Can't calculate Fn when n is less than or equal to zero.");
        }
        if (this.OrdinalPosition == 1 || this.OrdinalPosition == 2)
        {
            return 1;
        }

         //initialize previous results
        BigInteger previous1 = 1;
        BigInteger previous2 = 1;

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

Answer 2

It's nice to see that you've written a separate driver to use your Fibonacci class. Take care to spell "Fibonacci" consistently, though.

You seem to have a lot of superfluous using statements.

I wouldn't consider it an error to ask for \$F_0\$, which I would expect to return 0.

Your loop

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

would be better written as

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

both because the code is simpler, and because in a sense, you aren't starting from the beginning (\$F_0\$).

Your implementation could support an optimization for a common use case, which is to enumerate the Fibonacci sequence in order. Currently you start calculating from scratch based on \$F_1\$ and \$F_2\$ every time, which is quite inefficient. You should be able to implement such an optimization by tweaking the Fibonacci class, without touching the driver at all.

To answer your question about inheritance, you should not make Fibonacci a subclass of BigInteger. It's not reasonable to say that the Fibonacci Sequence is a BigInteger. Rather, the Fibonacci Sequence uses BigInteger. The operations on the Fibonacci Sequence (namely traversal) are quite different from the operations supported by BigInteger (such as arithmetic).

Answer 3

I see that no one touched your console program. I dislike few things about WriteOkMesage(), WriteNotAnIntegerMessage, and WriteAFibbonacci. These methods don't bring much to the table. I think a better solution would be to create a ShowMessage(string message) method and a few const strings.

You repeat if (int.TryParse(Console.ReadLine(), out input)). I would prefer to create a new function which asks the user for an integer:

static void Main(string[] args)
{
    int n = AskForInteger("How many Fibbonacci numbers should I print?");
    WriteLotsOfFibbonacci(n);

    ShowMessage(Environment.NewLine + "Let's just pick one at random.");
    int m = AskForInteger("What nth number Fibbonacci would you like?");
    WriteAFibbonacci(m);

    ShowMessage("Press enter to close...");
    Console.ReadLine();
}

static int AskForInteger(string msg)
{
    ShowMessage(msg);
    int result;
    while (! int.TryParse(Console.ReadLine(), out result))
    {
        ShowMessage("That's not an integer! I can't process that.");
    }
    ShowMessage("Okay, you picked {0}", result);
    return result;
}

static void ShowMessage(string msg)
{
    Console.WriteLine(msg);
}

static void ShowMessage(string format, params object[] args)
{
    ShowMessage(String.Format(format, args));
}

I'm not sure if AskForInteger should show a question or "okay" message. It's probably a better idea to put it outside.