There is a real lack of communications with the user of the program. There is no error checking performed on the possible input. While the code really doesn't do that much it is too complex and very hard to maintain.
Communications With the User
Unless the user knows exactly how to call the program, the program will crash with no indication of what went wrong. This is because the code uses
argv for input and doesn't check that
argv actually exists. The following code would be better for the user of the program because it check that an
argv actually exists first before using it and reports an error if there is no
if (argc > 1)
loops = atoll(argv);
fprintf(stderr, "USAGE MESSAGE");
It is also unclear what is being printed in the following statement:
printf ("%llu" "%s" "%llu" "%s" "%0.9f" "%s" "%0.3f\n",
i, " ", cpoints, " ", approx, " ", cpu_time_used);
It might be better if the the previous statement was something like this:
printf ("i = %llu cpoints = %llu approx = %0.9f cpu_time_used = %0.3f\n",
i, cpoints, approx, cpu_time_used);
Note: Use the formatting capabilities of the
printf statement rather than using "%s" to print spaces. Rather than using the variable
i in the statement it would be more understandable to use the variable
loops should be the same at this point.
Declare and Initialize the Variables as Needed
C is not the nicest language to program in because there is no default value assigned to local variables so it is always best to initialize the variable when it is declared, each variable should be declared and initialized on its own line to make the code easier to read, write, maintain and debug.
unsigned long long loops = 0;
unsigned long long cpoints = 0;
unsigned long long i = 0;
If the variable
loops is used for the calculations and printing then the variable
i can actually be declared in the for loop.
for (unsigned long long i = 0; i < loops; ++i)
double x = (rand() + 1.0) / (RAND_MAX+2.0);
double y = (rand() + 1.0) / (RAND_MAX+2.0);
if ( incircle(x,y) !=FALSE )
stdbool.h is included the
FALSE are unnecessary because
false. In the if statement where
FALSE is used you have a double negative, which is as bad in programming as it is in English. The if statement can be simplified to
cpoints += 1;
main() is too complex (does too much). As programs grow in size the use of
main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.
There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:
that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.
main() function can be broken up into the following functions
- Get the user input from the command line and report any errors
- Set up the random number generator.
- The loop that is currently doing the calculations should be in its own function.
Make sure you enable the compiler warnings when you compile some of the casts in the code are questionable and my compiler is reporting warnings. It might be best to compile with the -wall switch (enable all warnings).