Find the greatest number in an array

Question

The program reads a .txt file that contains lines of numbers. The first one has one number N and the second one has \$N\$ numbers as the first line says.

The \$N\$ numbers is \$1 \le N \le 1.000.000\$.

The program read the \$N\$ numbers in an array and performs the following procedure:

Read the number one by one backwards in the array and check to find the max value. Every time a new max value is found the int total increases by one. In the end we write the total value in an output file.

Is my code well written?

#include <stdio.h>
#include <stdlib.h>

/*---------------------*/
/* defined constants   */
/* for restriction     */
/*---------------------*/
#define MIN 1
#define MAX 1000000

/*---------------------*/
/* function prototypes */
/*---------------------*/
int main(void);
FILE *read_input(char *filename_r);
int count_children(FILE *input);
int pass_heights(FILE *input, int *children,const unsigned int size);
int chck_tall(const int *children,const unsigned int size);
void write_output(const unsigned short total,char *filename_w);


/*----------------------------------*/
/* start of program - Main Function */
/*----------------------------------*/
int main(void) {

    char *filename_r = "xxx.in";
    char *filename_w = "xxx.out";

    FILE *input = read_input(filename_r);

    unsigned int size = count_children(input);

    int *children = malloc(size * sizeof(unsigned short));
    if (children==NULL)
    exit(1); //General application error

    pass_heights(input, children, size);

    fclose(input);

    unsigned short total = chck_tall(children, size);

    free(children);

    write_output(total,filename_w);

    return 0;
}

FILE *read_input(char *filename_r) {

    FILE *input = fopen(filename_r, "r");

    if(input == NULL)
    exit(66); //EXIT_NOINPUT 'cannot open input'

    return input;
}

int count_children(FILE *input) {

    unsigned int count = 0;
    fscanf(input, "%d",&count);

    if(count > MAX || count < MIN)
    exit(1); //General application error

    return count;
}

int pass_heights(FILE *input, int *children,const unsigned int size) {

    for(register int i = 0; i < size; i++)
    fscanf(input, "%d",&children[i]);

    return *children;
}

int chck_tall(const int *children,const unsigned int size) {

    unsigned short total = 0;
    unsigned short tmp_max = 0;
    for(register int i = size - 1; i >= 0; i--)
    {
        if(children[i] > tmp_max) {
            tmp_max = children[i];
            total++;
        }
    }
    return total;
}


void write_output(const unsigned short total,char *filename_w) {

    FILE *output = fopen(filename_w, "w");

    if(output == NULL)
    exit(74); //EXIT_IOERR 'input/output error'

    fprintf(output, "%d\n", total);
    fclose(output);
}

Answer 1

1. Code should not mix pointer types. On systems where sizeof(unsigned short) < sizeof(int), this is a program flaw.

   // int *children = malloc(size * sizeof(unsigned short));
   int *children = malloc(size * sizeof(int));
   // or even better
   int *children = malloc(size * sizeof *children);
   

2. For high portability, use type size_t or unsigned long for an array index that may exceed 65,535:

   // unsigned int size = count_children(input);
   size_t size = count_children(input);
   int *children = malloc(size * sizeof *children);
   

3. Code should check the return value from fscanf(input, "%d",&count); and fscanf(input, "%d",&children[i]);

4. Mixing types for finding the maximum is very questionable. Suggest re-write

   size_t chck_tall(const int *children, size_t size) {
     size_t total = 0;
     int tmp_max = 0;   // or maybe INT_MIN
     for(size_t i = size - 1; i-- > 0; )  {
       if(children[i] > tmp_max) {
         tmp_max = children[i];
         total++;
       }
     }
     return total;
   }
   

Answer 2

1. Constant string literals should be declared as such (const):

   const char *filename_r = "xxx.in";
   const char *filename_w = "xxx.out";
   

   If you don't do that then you have a char * pointing to a string literal which the compiler will probably put in a read only data segment. Should you ever try to modify it (via the non-const char pointer) you'll run into undefined behaviour (most likely it will crash your program).

2. Your helper methods should in general not terminate the program (you call exit() in quite a few places). Their return value should indicate that they could not perform the operation they were supposed to leaving it to the caller (in your case main) to take appropriate action (like terminating early).

3. You should alway places braces around blocks, even one-liners. Things like this:

   if(count > MAX || count < MIN)
   exit(1); //General application error
   

   are really confusing as it reads like exit() is executed unconditionally and if you ever add another statement to it then you'll run into trouble. Braces make it crystal clear what's intended:

   if (count > MAX || count < MIN)
   {
       exit(1); //General application error
   }
   

   This happens in quite a few places. Readability beats brevity anytime.

4. You use unsigned short for the count which means it will be unsigned 16bit on most platforms. So if the input sequence is descending like 100000, 99999, 99998, ..., 1 with more than 65535 elements then your count variable will overflow.

5. Don't leave vowels out of names for functions or variables: chck_tall vs check_tall. If you leave them out and they do not shorten the name sufficiently then why did you leave them out. And if they shorten the name sufficiently then it's likely it will become too garbled to read properly. So in either case leaving them out is not a wise choice.

6. Your array holds int yet when you check for the max in chck_tall your temporary variables are short which can lead to truncation. Your compiler should have warned you about it. Do not ignore compiler warnings. In fact it is not uncommon in production code to run with an option which treats warnings as errors (most compilers do offer such an option).

7. Not sure why you think you need to make the loop variable register:

   for (register int i = size - 1; i >= 0; i--)
   

   register is just a hint to the compiler which is free to ignore it anyway. Typically you should only use it in specific circumstances when you know if and why it is required. Otherwise it's just adding noise.

Answer 3

It is good to always use register for loop variables as it may indeed be used, especially for small console programs. I would have declared it above, a personal preference, as people like to see declarations after the opening brace (= stack lift).

Having meaningful codes for exit() is good, and for utility work like this it is OK to drop out in this manner. (Keeping it simple).

A more sophisticated extension would be to define values. So instead of :

exit(74);

one might have

exit(IOERROR);

say. This is in case you want to renumber them in the future or re-use them from a common header file used in your shop.

I know I have said keep it simple, but your code may be copied by a peer. It is always good to close files independently of the writing of any output.

In your case you are writing once, fair enough. But a common issue I have seen raging unchecked is the habit of closing files after every file write, and whilst this forces writes to disk it can result in a huge 'seek' overhead for millions of records when the file is re-opened for append upon each new write.

Maybe close the output file in main().