Find the greatest number in an array

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
#include

/---------------------/
/ 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 = 0; i--)
{
if(children[i] >

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

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

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

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

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

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

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

StackExchange Code Review Q#77257, answer score: 6