C - approximate pi (Monte Carlo)

Question

This is my first C program that is not "Hello World" level; I would welcome ideas to make it more efficient and readable.

#include <stdio.h>    
#include <stdlib.h>
#include <math.h>
#include <time.h>  
#include <stdbool.h>
#define FALSE 0
#define TRUE  1

bool incircle(double x, double y);

int main (int argc, char *argv[]) {
    unsigned long long loops, cpoints, i;
    double             approx, cpu_time_used;
    int                seed;
    clock_t start, end;
    cpoints  = 0;
    loops = atoll(argv[1]);
    seed = time(NULL);
    srand(seed);
    start = clock();
    for (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 )
            cpoints+=1;
    }
    end = clock();
    cpu_time_used = ((double) (end - start)) / CLOCKS_PER_SEC;
    approx=4*(cpoints/(double)i);
    printf ("%llu" "%s" "%llu"   "%s" "%0.9f" "%s" "%0.3f\n",
            i,     " ", cpoints, " ", approx, " ", cpu_time_used);
    return 0;
}

bool incircle(double x, double y) {
        return ( x * x + y * y <= 1 );
    }
    

Answer 1

General Observations

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[1] for input and doesn't check that argv[1] actually exists. The following code would be better for the user of the program because it check that an argv[1] actually exists first before using it and reports an error if there is no argv[1]:

    if (argc > 1)
    {
        loops = atoll(argv[1]);
    }
    else
    {
        fprintf(stderr, "USAGE MESSAGE");
        return EXIT_FAILURE;
    }

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, i and 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 )
            cpoints+=1;
    }

Unnecessary Code

Since stdbool.h is included the #defines for TRUE and FALSE are unnecessary because stdbool.h defines true and 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

        if (incircle(x,y))
            cpoints += 1;

Code Complexity

##Complexity The function 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.

The main() function can be broken up into the following functions

1. Get the user input from the command line and report any errors
2. Set up the random number generator.
3. The loop that is currently doing the calculations should be in its own function.

Compiler Warnings

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).