Reading, counting, and processing integers from a text file

The following code reads a .txt file that contain only numbers. Read the first one, create a dynamic array and put the other numbers on it, made some procedures and in the end write a file with the int total.

I want to know if there is a way to run the code faster (the maximum procedure time should be 1s) if I had a .txt file with 1,000,000 numbers.

Is there a more stable way to write this code? Do I have to take all the restrictions I could?

```
#include
#include
#include
static int total = 1;

int process( int my_array, int endp) {
int a, b;
//static int total = 1;
if ( my_array == 0 ) return 0;
if ( my_array == endp ) return INT_MIN;
else a = *my_array++;
if ( (b= process( my_array, endp )) == INT_MIN ) return a;
if ( a > b )
{total++; return printf( "%d > %d and total now is %d\n", a, b, total ), a; }
return b;
}

int main ()
{
int t_size;

FILE * fp;
fp = fopen ("xxx.txt", "r");
if (fp == NULL) // error here if there was a problem!
{
printf("Opening 'xxx.txt file failed: %s\n",strerror(errno)); // No such file or directory
getchar(); // wait the user press a key
return 0; // returning an int of 0, exit the program
}
else {

fscanf(fp, "%d", &t_size);

int readch = 0;
long int filepos = 0L;
filepos = ftell ( fp); // get the file position
while ( ( readch = fgetc ( fp)) != EOF) { // read each character of the file
if ( isalpha ( readch)) { // check each character to see if it is a letter
printf ( "File contains letters\n");

int process( int *my_array, int *endp) { 
    int a, b;

My first comment is that process is a weak name for a function. Almost all functions could be said to be processing their parameters. A more descriptive name would make it easier to see what the function is doing.

//static int total = 1;

You should avoid commented code in production. By the time that you are ready to send out for code review, you should have cleaned these things out of the code.

if ( my_array == 0 ) return 0;

OK, if the array is invalid (a null pointer), return 0. You are doing this check for every recursive call of the function, but this could only happen in the first call. You should actually do this check outside of the process function (looking in main, you already do this) and ensure that you always pass a valid array pointer into the function. That will save you a comparison on every recursive call. Get rid of this call.

Note that the comparison to NULL that you have in main is the correct way to do it. The 0 check will likely work (because NULL is almost always set to 0), but it is less reliable.

if ( my_array == endp ) return INT_MIN;
else a = *my_array++;

The function is starting to do something here. If the array is at the end, you return the minimum possible integer, which will be a negative number. If the array is not at the end, you grab the first value of the array and move one further back into the array.

if ( (b= process( my_array, endp )) == INT_MIN ) return a;

Here's the recursive call. You get the result of process on the new smaller array and check if it's equal to INT_MIN. This could happen either because every remaining value in the array is equal to INT_MIN or because there's nothing left in the array. This seems unnecessary. I'm thinking that we could get rid of the comparison and return here. Obviously, the recursive call is necessary.

if ( a > b ) 
{total++; return printf( "%d > %d and total now is %d\n", a, b, total ), a; }

This is overly complicated and more easily written:

if ( a > b ) 
{
    total++;
    printf("%d > %d and total now is %d\n", a, b, total);
    return a;
}

If a is greater than the recursive result of calling this function on the rest of the array: increment a total variable; print out a statement describing what happened; return a because it is the largest member of my_array.

return b; 
}

Finally, if b is greater than or equal to a, you return b. We can now say that what this function does is: it sets a global variable to the number of times that an element of the array is greater than all the members to its right; it returns the largest value from the array; and it produces output. That's three things done by one function, but the usual preference is to only do one thing per function.

You mention that you are worried about efficiency. So apparently you put these three responsibilities into one function so as to maximize efficiency. I'll accept that decision. Perhaps someone else wants to argue it, but I think that there's some things that I can tell you within the parameters that you set.

There's basically two things in this function that eat up time. One is that you do a lot of print statements (up to one less than the number of inputs). The other is that you are doing the recursive call. Presumably you have to do the print statements, so we're stuck with those. We don't have to do the recursive call.

int process( int *my_array, int *endp) {
    // this function should only be called if there is at least one value in the array
    int maximum = *(--endp);

    while ( endp > my_array ) {
        int a = *(--endp);

        if ( a > maximum ) {
            total++;
            printf("%d > %d and total now is %d\n", a, b, total);
            maximum = a;
        }
    }

    return maximum;
}

This function sets the maximum to the last element of the array and then updates it as necessary. It iterates over the array from end to beginning. If the current element is greater than the maximum, it updates maximum to be the current element. Because we know that we are starting at the end, we can save most of the checks that we are at or near the end in favor of one check whether we've reached the beginning. Further, because maximum is always maintained from the end, we only have to update when it changes. If the maximum is greater than or equal to the current element, we can simply do nothing.

This function eliminates the need to do any recursive calls. It's purely iterative.

This review seems long enough, so I'll let someone else review your main function. I'll just note that it would be easier to read if you reformatted the indentation so that code inside a block was consistently indented more than the code outside the block. Consistency in formatting makes code easier to read.

StackExchange Code Review Q#70078, answer score: 6