Is this good OOP?

[Protoman]

Is this a good way of making use of OOP? Also, is this a good way of
computing Fibonacci numbers?

Code:

#include <iostream>
#include <cstdlib>
using namespace std;

template <class T>
class invalidArg
{
public:
virtual void Write(T& arg)const{cout << arg << " is invalid. " <<
endl;}
};

class fib
{
public:
unsigned long operator()(unsigned long n)
{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n==1||n==2)
return 1;
unsigned long a=1, b=0;
unsigned long i=0x8000;
while (i)
{
unsigned long aa;
aa=n&i?(2*b+a)*a:a*a+b*b;
b=n&i?a*a+b*b:b*(2*a-b);
a=aa;
i>>=1;
}
return b;
}
};

class mainObj
{
public:
static mainObj* Instance()
{
if(!instanceFlag)
{
main=new mainObj();
instanceFlag=true;
return main;
}
else
return main;
}
~mainObj()
{
instanceFlag=false;
}
private:
mainObj()
{
for(;
{
cout << "Enter a number: " << endl;
unsigned long x;
cin >> x;
try
{
cout << "Value of fibonacci number: " << fib()(x) << endl;
}
catch(invalidArg<unsigned long>& inv)
{
inv.Write(x);
}
catch(...)
{
cout << "An error has occurred. Please contact Microsoft for
assistance. "
<< endl;
}
}
}
static bool instanceFlag;
static mainObj *main;
};
bool mainObj::instanceFlag = false;
mainObj* mainObj::main=0;
namespace{mainObj* Main=mainObj::Instance();}
int main(){system("PAUSE"); return 0;}

Is this good OOP? Thanks.

 

[Dave Townsend]

Is this a good way of making use of OOP? Also, is this a good way of
computing Fibonacci numbers?

Code:

#include <iostream>
#include <cstdlib>
using namespace std;

template <class T>
class invalidArg
{
public:
virtual void Write(T& arg)const{cout << arg << " is invalid. " <<
endl;}
};

class fib
{
public:
unsigned long operator()(unsigned long n)
{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n==1||n==2)
return 1;
unsigned long a=1, b=0;
unsigned long i=0x8000;
while (i)
{
unsigned long aa;
aa=n&i?(2*b+a)*a:a*a+b*b;
b=n&i?a*a+b*b:b*(2*a-b);
a=aa;
i>>=1;
}
return b;
}
};

class mainObj
{
public:
static mainObj* Instance()
{
if(!instanceFlag)
{
main=new mainObj();
instanceFlag=true;
return main;
}
else
return main;
}
~mainObj()
{
instanceFlag=false;
}
private:
mainObj()
{
for(;
{
cout << "Enter a number: " << endl;
unsigned long x;
cin >> x;
try
{
cout << "Value of fibonacci number: " << fib()(x) << endl;
}
catch(invalidArg<unsigned long>& inv)
{
inv.Write(x);
}
catch(...)
{
cout << "An error has occurred. Please contact Microsoft for
assistance. "
<< endl;
}
}
}
static bool instanceFlag;
static mainObj *main;
};
bool mainObj::instanceFlag = false;
mainObj* mainObj::main=0;
namespace{mainObj* Main=mainObj::Instance();}
int main(){system("PAUSE"); return 0;}

Is this good OOP? Thanks.

No, sorry I don't see any OOP at all in this example, its just uses a lot of
various unnecessary C++ features. You could have done the whole thing much
simpler as a simple Fibonacci number generator function.

Perhaps you could consider a Fibonacci object which would generate Fibonacci
numbers efficinetly
by caching the intermediate numbers when calculating a new value. In that
case this would have some
OOP, the object would have some internal state.

dave

 

[Protoman]

What do you mean? It uses classes and stuff.

 

[Greg]

Is this a good way of making use of OOP? Also, is this a good way of
computing Fibonacci numbers?

Code:

#include <iostream>
#include <cstdlib>
using namespace std;

template <class T>
class invalidArg
{
public:
virtual void Write(T& arg)const{cout << arg << " is invalid. " <<
endl;}
};

class fib
{
public:
unsigned long operator()(unsigned long n)
{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n==1||n==2)
return 1;
unsigned long a=1, b=0;
unsigned long i=0x8000;
while (i)
{
unsigned long aa;
aa=n&i?(2*b+a)*a:a*a+b*b;
b=n&i?a*a+b*b:b*(2*a-b);
a=aa;
i>>=1;
}
return b;
}
};

class mainObj
{
public:
static mainObj* Instance()
{
if(!instanceFlag)
{
main=new mainObj();
instanceFlag=true;
return main;
}
else
return main;
}
~mainObj()
{
instanceFlag=false;
}
private:
mainObj()
{
for(;
{
cout << "Enter a number: " << endl;
unsigned long x;
cin >> x;
try
{
cout << "Value of fibonacci number: " << fib()(x) << endl;
}
catch(invalidArg<unsigned long>& inv)
{
inv.Write(x);
}
catch(...)
{
cout << "An error has occurred. Please contact Microsoft for
assistance. "
<< endl;
}
}
}
static bool instanceFlag;
static mainObj *main;
};
bool mainObj::instanceFlag = false;
mainObj* mainObj::main=0;
namespace{mainObj* Main=mainObj::Instance();}
int main(){system("PAUSE"); return 0;}

Is this good OOP? Thanks.

Well, I think you got a bit carried away with the object-oriented
programming. Not everything is improved by turning it into an object.
For instance, the function "main" does not really lend itself to an
object-oriented model. Therefore MainObj (and, by the way, let's buy a
vowel and ask Vanna for some consonants so that we can use complete
words as names: MainObject instead of MainObj, for example). (And one
other tip along those lines: there is a "tab" key on your keyboard.)

There are two problems with MainObj: the first is that practically the
entire program executes within MainObj's constructor. First, no one is
going to know to look there to find the program. Everyone will look
first at main(). Otherwise, there is no convention of having a
"MainObj" with which other programmers will be familiar. Imagine if
this program had 50 classes and 50 source files, who would want to look
through them all wondering where the main routine really is.

The other problem is that executing the program before main() means
that global objects in other files may not yet be initialized when
MainObj's constructor is called; subtle bugs can occur as a result. In
short, "main" exists for a reason - it is the first function executed
after everything has been set up, and a function with which every C and
C++ programmer will look for. A program should take advantage of both
of those facts.

Greg

 

[Protoman]

OK then, how's my fib() funct? Is it efficient?

 

[John Harrison]

What do you mean? It uses classes and stuff.

Exactly, it uses classes where they are totally unnecessary. Here's a
tip, if you write a class that has no data members at all you should
consider whether you need to make that a class at all.

Here one valid possibility for OOP fibonacci numbers. Classes like the
following are called generators.

class FibGenerator
{
public:
FibGenerator() : a(0), b(1) {}
int operator()
{
int res = a + b;
a = b;
b = res;
return res;
}
private:
int a;
int b;
};

int main()
{
FibGenerator fib;
for (int i = 0; i < 10; ++i)
cout << fib() << '\n';
}

Untested code.

john

 

[John Harrison]

Here one valid possibility for OOP fibonacci numbers. Classes like the
following are called generators.

And here's a version that actually works.

#include <iostream>
using namespace std;

class FibGenerator
{
public:
FibGenerator() : a(0), b(1) {}
int operator()()
{
int next = a + b;
a = b;
b = next;
return a;
}
private:
int a;
int b;
};

int main()
{
FibGenerator fib;
for (int i = 0; i < 10; ++i)
cout << fib() << '\n';
}

 

[Dave Townsend]

What do you mean? It uses classes and stuff.

You code is neither good or OOP just because it uses classes and stuff.
You've used a
number of advanced C++ language features very ineffectively:

1. Your fibonacci class has no data which supports the computation of
Fibonacci
numbers, so you might as well use a simple free function for that.
Usually objects
have their own state and identity otherwise they aren't very interesting
or useful

2. You have introduced an invalidArg class template although only one type
is used in
the program. The invalidArg class also writes to cout which makes the
class less useful
and reuseable. ( eg, if I write a windows GUI app, writing to cout is
not a useful thing to do, or
what if I want to write to stderr ? ). Throw the exception by all
means, but let the client deal with the exception himself. Put the
information about the exception into the exception class and let your client
deal with it.

3. Only one exception can be thrown from your fibonacci generator, so
perhaps that should be
specified in the signature. You've put in a catch(...) which is
desperation, you should check carefully
that your fibonacci calculation can never throw exceptions and then
remove the catch(...).

You've not done anything to guard against arithmetic overflow when the
Fibonacci number is too
big to be stored in a long.

4. The mainObj serves no useful purpose and just adds a lot of complexity
over the simple main()
function.

Here's my improved version with some of the suggestions.....

#include <iostream>
#include <cstdlib>
using namespace std;

template <class T>
class invalidArg
{
public:
virtual void Write(T& arg)const{cout << arg << " is invalid. " << endl;}
};


unsigned long Fibonacci( unsigned long n)
{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n==1||n==2)
return 1;

unsigned long a=1, b=0;
unsigned long i=0x8000;
while (i)
{
unsigned long aa;
aa=n&i?(2*b+a)*a:a*a+b*b;
b=n&i?a*a+b*b:b*(2*a-b);
a=aa;
i>>=1;
}
return b;
}

void doFibonacci()
{
for ( ; ; )
{
cout << "Enter a number: " << endl;
unsigned long x;
cin >> x;
try
{
cout << "Value of fibonacci number: " << Fibonacci( x) << endl;
}
catch( invalidArg<unsigned long>& inv)
{
inv.Write(x);
}
}
}


int main( )
{

doFibonacci();
system("PAUSE");
return 0;
}

 

[Mike Wahler]

What do you mean? It uses classes and stuff.

'Classes and stuff', in themselves, do not OOP make.

-Mike

 

[Greg]

OK then, how's my fib() funct? Is it efficient?

Well, let's critique it. First again is the chosen name. In this case,
"fib" is a English word - to fib means to lie, usually about small
things. As such, "fib" is not a great name for a class that is supposed
to perform a calculation and return an (accurate) result. This class
should be named for it does, GetNthFibonacciNumber or similar.

class fib
{
public:
unsigned long operator()(unsigned long n)
{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n==1||n==2)
return 1;

unsigned long a=1, b=0;
unsigned long i=0x8000;

while (i)
{
unsigned long aa;
aa=n&i?(2*b+a)*a:a*a+b*b;
b=n&i?a*a+b*b:b*(2*a-b);
a=aa;
i>>=1;
}
return b;
}
};

Let's look at the declaration:

unsigned long operator()(unsigned long n)

Because this function is an overloaded operator, there is nothing,
another than the name of the class, "fib", to indicate what this
function does or what the "n" parameter is used for. Again, using a
named function and parameters with descriptive names would help a lot.

Let's look at these lines:

if(n<=0)
throw invalidArg<unsigned long>();
else if(n==1||n==2)
return 1;

First, we don't need the "else" since an n less than or equal to zero
will exit the routine at once. Next, since the program has eliminated
all negative numbers and zero, the if clause can be consolidated:

if(n<=0)
throw invalidArg<unsigned long>();
if(n <= 2)
return 1;

Now to the calculation itself:

unsigned long a=1, b=0;
unsigned long i=0x8000;

while (i)
{
unsigned long aa;

aa = n & i ? (2*b+a) * a : a*a+b*b;
b = n & i ? a*a+b*b: b * (2*a-b);
a = aa;
i >>= 1;
}
return b;

Now whatever this routine is doing it does look clever. But I have no
idea if it is correct, since it is not how one generally thinks of
Finbonacci numbers 0 + 1 + 1 + 2 + 3 + 5 + 8.... And again, the cryptic
variable names a, b, aa, i do not help. What I would like to see is
some comment explaining where this algorithm came from - a reference
that I could then look up to reassure myself that this code is correct
and for me to better understand it.

Now as for its efficiency: note that n & i is evaluated twice, so there
is some small room for improvement:

while (i)
{
unsigned long aa;

if (n & i)
{
aa = (2*b+a) * a ;
b = a*a+b*b;
}
else
{
aa = a*a+b*b;
b = b * (2*a-b);
}
a = aa;
i >>= 1;
}
return b;

A bigger problem is the upper limit for n. It also appears that only
the first 46 or so Fibonacci numbers can fit in 32 bits. So any
calculation with n>46 and 32 bit longs will return the wrong result. In
other words, the routine will fib.

Greg

 

[Dave Townsend]

OK then, how's my fib() funct? Is it efficient?

Your function isn't efficient either....this is the comment in the Wiki
article where your code
appears....

It is essentially equivalent to the versions that employ matrix arithmetic,
but with redundant calculations eliminated. Note that this method only makes
sense for high-precision calculations where the benefit of an O(log n)
algorithm outweighs the extra complexity. Since F(48) exceeds 232 - 1, no
ordinary C program with 32-bit integers can calculate F(n) for n ? 48, and
the sophisticated algorithm in this version is just over-engineering.


Since your algorithm is only valid for the first 48, you can precompute
these values ahead of time and store them in a table.

 

[John Harrison]

Your function isn't efficient either....this is the comment in the Wiki
article where your code
appears....

Ouch!! I do hope Protoman wasn't going to pass that off as his own code.

john

 

[John Harrison]

OK then, how's my fib() funct? Is it efficient?

If you want to generate a Fib(n) for an arbitrary value of n then it is
efficient.

On the other hand if you want to generate the series of Fibonacci
numbers, Fib(1), Fib(2), Fib(3), etc. then it is *extremely* inefficient.

So the answer to depends on what you actually want to do with Fibonacci
numbers. Something you probably haven't worked out for yourself yet.

john

 

[Protoman]

Your function isn't efficient either....this is the comment in the Wiki
article where your code
appears....

It is essentially equivalent to the versions that employ matrix arithmetic,
but with redundant calculations eliminated. Note that this method only makes
sense for high-precision calculations where the benefit of an O(log n)
algorithm outweighs the extra complexity. Since F(48) exceeds 232 - 1, no
ordinary C program with 32-bit integers can calculate F(n) for n ? 48, and
the sophisticated algorithm in this version is just over-engineering.


Since your algorithm is only valid for the first 48, you can precompute
these values ahead of time and store them in a table.

OK, here's the new algorithm; it works for over 48 now:

{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n<=2)
return 1;
unsigned long long a=1, b=0;
unsigned long long i=0x8000;
while (i)
{
unsigned long aa;
if (n&i)
{
aa=(2*b+a)*a;
b=a*a+b*b;
}
else
{
aa=a*a+b*b;
b=b*(2*a-b);
}
a=aa;
i>>=1;
}
return b;
}

And where should I put the try/catch statement?

 

[Protoman]

(And one ohe tip: there is a "tab key on your keyboard.)

WHY DO I need the tab key for?

 

[Protoman]

Your function isn't efficient either....this is the comment in the Wiki
article where your code
appears....

It is essentially equivalent to the versions that employ matrix arithmetic,
but with redundant calculations eliminated. Note that this method only makes
sense for high-precision calculations where the benefit of an O(log n)
algorithm outweighs the extra complexity. Since F(48) exceeds 232 - 1, no
ordinary C program with 32-bit integers can calculate F(n) for n ? 48, and
the sophisticated algorithm in this version is just over-engineering.


Since your algorithm is only valid for the first 48, you can precompute
these values ahead of time and store them in a table.

OK then, but is it fast?

 

[Greg]

WHY DO I need the tab key for?

Indentation. Improves readability and helps to verify correctness. In
other words, this code:

{
if(n<=0)
throw invalidArg<unsigned long>();
else if(n<=2)
return 1;
unsigned long long a=1, b=0;
unsigned long long i=0x8000;
while (i)
{
unsigned long aa;
if (n&i)
{
aa=(2*b+a)*a;
b=a*a+b*b;

}

should look more like:

{
if (n <= 0)
throw invalidArg<unsigned long>();
else
if (n <= 2)
return 1;

unsigned long long a=1, b=0;
unsigned long long i=0x8000;

while (i)
{
unsigned long aa;

if (n&i)
{
aa = (2*b+a)*a;
b = a*a+b*b;
}
}

Although I use the space bar for indenting code posted in messages.

Greg