Prev: bitwise operator
Next: Can't get boost::program_options to work.
From: edwardzyang on 7 Dec 2006 20:55
This is the second program (not counting Hello World) that I have ever
written in C++. However, I'm trying to learn the language while picking
up as few bad habits as possible, so please be nit-picky!

/**
* Exercise #2.1
*
* Create a program that takes one argument and prints the
corresponding
* term from the Fibonnaci series. Bonus: implement caching for
function
* returns. It also accepts a -test flag which prints a list of
fibonnaci
* numbers.
*/

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

/**
* Defines a whole number type, an integer that is non-negative. This
can
* be arbitrarily large: the number of bytes allocated per each number
* determines how why the numbers we calculate can be before it rolls
* over.
*/
typedef unsigned long WholeNumber;

/**
* Lookup cache for return values to the fibonacci() function.
*/
map<WholeNumber, WholeNumber> fibonnaci_cache;

/**
* Recursively calculates a term of the Fibonnaci series.
*/
WholeNumber fibonnaci(WholeNumber term) {
if (term <= 0) return 0;
if (term == 1) return 1;
if (fibonnaci_cache.find(term) != fibonnaci_cache.end()) {
return fibonnaci_cache[term];
}
WholeNumber result = fibonnaci(term - 2) + fibonnaci(term - 1);
fibonnaci_cache[term] = result;
return result;
}

int main(int argc, char* argv[]) {
if (argc == 1) {
// no arguments, give usage info
cout << "Usage: fibonnaci.exe [-test] num \n";
return 0;
}
if (strcmp(argv[1], "-list") == 0) {
// -test flag present, output a list
int max = (argc == 3) ? atoi(argv[2]) + 1 : 20;
for (int i = 1; i < max; i++) {
cout << fibonnaci(i);
if (i != max - 1) cout << ", ";
}
return 0;
}
// normal execution
WholeNumber term = atoi(argv[1]);
cout << fibonnaci(term);
return 0;
}

From: Neal E. Coombes on 7 Dec 2006 22:30
On Dec 7, 7:55 pm, edwardzy...(a)thewritingpot.com wrote:
> This is the second program (not counting Hello World) that I have ever
> written in C++. However, I'm trying to learn the language while picking
> up as few bad habits as possible, so please be nit-picky!
>
> /**
> * Lookup cache for return values to the fibonacci() function.
> */
> map<WholeNumber, WholeNumber> fibonnaci_cache;

This map should probably be a static local inside the fibonnaci
function since it has no other use. Both the global and the static
local could cause reentrance problems for the function, so both being
equal in that respect it is better to limit the scope of this variable.

> int main(int argc, char* argv[]) {
> if (argc == 1) {
> // no arguments, give usage info
> cout << "Usage: fibonnaci.exe [-test] num \n";
> return 0;
> }
> if (strcmp(argv[1], "-list") == 0) {
> // -test flag present, output a list
> int max = (argc == 3) ? atoi(argv[2]) + 1 : 20;
> for (int i = 1; i < max; i++) {
> cout << fibonnaci(i);
> if (i != max - 1) cout << ", ";
> }
> return 0;
> }
> // normal execution
> WholeNumber term = atoi(argv[1]);
> cout << fibonnaci(term);
> return 0;

What if I call 'fibonnaci --list' by accident? Try to think of edge
cases for your program, assume the user will always pass in something
you don't expect. You should likely print the usage in all unexpected
cases. The usage would be more useful if it had a description of what
the program does exactly, or at least a -help flag which would give
that extra detail.

You could use std::string instead of comparing C-strings directly (not
a huge win in this case, but a fairly good habit). I also would
recommend looking into boost::lexical_cast over atoi (it's only
downfall is with floating points). For fun you could look at getopt or
boost::program_option for standardized command line option parsers.

Also, find and replace all occurances of 'fibonnaci' with 'fibonacci'
;)

HTH
Neal

From: BobR on 8 Dec 2006 03:25

edwardzyang(a)thewritingpot.com wrote in message ...
>This is the second program (not counting Hello World) that I have ever
>written in C++. However, I'm trying to learn the language while picking
>up as few bad habits as possible, so please be nit-picky!

OK.

>
>/**
> * Exercise #2.1
> *
> * Create a program that takes one argument and prints the
>corresponding
> * term from the Fibonnaci series. Bonus: implement caching for
>function
> * returns. It also accepts a -test flag which prints a list of
>fibonnaci
> * numbers.
> */

You're using C multiline comments in a C++ program. Yes, I'm kidding. Is that
picky enough?

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

Now, that's serious! You said you wanted to 'pick up as few bad habits as
possible'. Here's one. Limit the amount of 'std' that you expose. and where
you expose it. See below.

>
>/**
> * Defines a whole number type, an integer that is non-negative. This
>can
> * be arbitrarily large: the number of bytes allocated per each number
> * determines how why the numbers we calculate can be before it rolls
> * over.
> */
>typedef unsigned long WholeNumber;

I hate typedefs, so, I'm gonna flag this one. You can use them if you want,
but, one purpose is to type less. You are only saving 2 chars. I'd re-name
that 'ULNum' or something shorter.

>
>/**
> * Lookup cache for return values to the fibonacci() function.
> */
>std::map<WholeNumber, WholeNumber> fibonnaci_cache;
>
>/**
> * Recursively calculates a term of the Fibonnaci series.
> */
>WholeNumber fibonnaci(WholeNumber term) {
> if (term <= 0) return 0;
> if (term == 1) return 1;
> if (fibonnaci_cache.find(term) != fibonnaci_cache.end()) {
> return fibonnaci_cache[term];
> }
> WholeNumber result = fibonnaci(term - 2) + fibonnaci(term - 1);
> fibonnaci_cache[term] = result;
> return result;
> }
>
>int main(int argc, char* argv[]) {
using std::cout;
using std::atoi;
using std::strcmp;

> if (argc == 1) {
> // no arguments, give usage info
> cout << "Usage: fibonnaci.exe [-test] num \n";
> return 0;
> }
> if (strcmp(argv[1], "-list") == 0) {
> // -test flag present, output a list
> int max = (argc == 3) ? atoi(argv[2]) + 1 : 20;
> for (int i = 1; i < max; i++) {
> cout << fibonnaci(i);
> if (i != max - 1) cout << ", ";
> }

// Got strings? Learn them early. Below is not what I call 'normal use'.
// I'm not saying replace the above with the below, just showing
// how I might do it.
// #include <string>
// #include <sstream> // std::istringstream

if( std::string( argv[ 1 ] ).find( "-list" ) != std::string::npos
&& ( argc >= 3 ) ){
// -test flag present, output a list
std::istringstream sis( argv[2] );
int max(0);
sis >> max;
if( not max ){
cout<<"Error!"<<std::endl;
max = 20;
}
for( int i(1); i <= max; ++i ){
cout << fibonnaci( i );
if (i != max - 1) cout << ", ";
} // if(i)

> return 0;
> }
> // normal execution
> WholeNumber term = atoi(argv[1]);
> cout << fibonnaci(term);
> return 0;
> }
>

--
Bob R
POVrookie


From: Mark P on 8 Dec 2006 04:18
edwardzyang(a)thewritingpot.com wrote:
> This is the second program (not counting Hello World) that I have ever
> written in C++. However, I'm trying to learn the language while picking
> up as few bad habits as possible, so please be nit-picky!
>
> /**
> * Exercise #2.1
> *
> * Create a program that takes one argument and prints the
> corresponding
> * term from the Fibonnaci series. Bonus: implement caching for
> function
> * returns. It also accepts a -test flag which prints a list of
> fibonnaci
> * numbers.
> */
>
> #include <iostream>
> #include <map>
> using namespace std;
>
> /**
> * Defines a whole number type, an integer that is non-negative. This
> can
> * be arbitrarily large: the number of bytes allocated per each number
> * determines how why the numbers we calculate can be before it rolls
> * over.
> */
> typedef unsigned long WholeNumber;
>
> /**
> * Lookup cache for return values to the fibonacci() function.
> */
> map<WholeNumber, WholeNumber> fibonnaci_cache;

A map doesn't really make sense here. Your algorithm guarantees that if
you need to look up index i, then you also need to look up all indices
less than i. Since your indices are in consecutive numerical order, a
vector would be more appropriate and probably faster too.

>
> /**
> * Recursively calculates a term of the Fibonnaci series.
> */
> WholeNumber fibonnaci(WholeNumber term) {
> if (term <= 0) return 0;

An unsigned type should never compare less than zero. Moreover, if a
negative term were permitted to be entered, the program should emit an
error.

> if (term == 1) return 1;
> if (fibonnaci_cache.find(term) != fibonnaci_cache.end()) {
> return fibonnaci_cache[term];
> }
> WholeNumber result = fibonnaci(term - 2) + fibonnaci(term - 1);
> fibonnaci_cache[term] = result;
> return result;
> }
>
> int main(int argc, char* argv[]) {
> if (argc == 1) {
> // no arguments, give usage info
> cout << "Usage: fibonnaci.exe [-test] num \n";
> return 0;
> }
> if (strcmp(argv[1], "-list") == 0) {
> // -test flag present, output a list
> int max = (argc == 3) ? atoi(argv[2]) + 1 : 20;
> for (int i = 1; i < max; i++) {
> cout << fibonnaci(i);
> if (i != max - 1) cout << ", ";
> }
> return 0;
> }
> // normal execution
> WholeNumber term = atoi(argv[1]);
> cout << fibonnaci(term);
> return 0;
> }
>
From: Richard Heathfield on 8 Dec 2006 06:07
BobR said:

>
> edwardzyang(a)thewritingpot.com wrote in message ...
>>This is the second program (not counting Hello World) that I have ever
>>written in C++. However, I'm trying to learn the language while picking
>>up as few bad habits as possible, so please be nit-picky!
>
> OK.
>
>>
>>/**
>> * Exercise #2.1
>> *
>> * Create a program that takes one argument and prints the
>>corresponding
>> * term from the Fibonnaci series. Bonus: implement caching for
>>function
>> * returns. It also accepts a -test flag which prints a list of
>>fibonnaci
>> * numbers.
>> */
>
> You're using C multiline comments in a C++ program.

Funny - they look remarkably like C++ multiline comments to me. If we're
going to be picky about them, it would be better to pick on how the
formatting is rendered rather inelegant by Usenet-quoting.

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at the above domain, - www.
 |  Next  |  Last
Pages: 1 2 3 4
Prev: bitwise operator
Next: Can't get boost::program_options to work.