Constructive criticism
Compile with warning level at maximum:
Code:
$ make foogcc -Wall -ggdb3 -pedantic -std=gnu99 -O0 -o foo foo.c -lm -lpthread -lrt
foo.c: In function ‘main’:
foo.c:10:5: warning: missing braces around initializer [-Wmissing-braces]
foo.c:10:5: warning: (near initialization for ‘temp[0]’) [-Wmissing-braces]
For every nested aggregate type (array/multi-dimensional array, union, struct), you should have a set of curly braces for the initializer, delineating the sub-objects:
Code:
int temp[ROWS][COLUMNS] = {{0}};
Check all your file function (fopen, fscanf) for errors. If encountered, print a useful message (look into perror or strerror functions). If need be, exit (if you can't read the data, no point in continuing).
Don't use global variables (inptr). Read this link. Instead, declare the variable in the appropriate function and pass it to other functions as needed -- this is trivial for you, you only have a main function.
Avoid all magic numbers/unnamed constants. Also, pick names that describe what the data really represents. I used ROWS and COLS because I didn't look at what the data represented (shame on me, being so lazy/careless). A better name for ROWS would be NUM_DAYS. A better name for COLS might be NUM_TEMPS. You could create an enum that describes what each column is:
Code:
enum {
TEMP_MIN,
TEMP_MAX,
TEMP_AVG,
NUM_TEMPS
};
Also, define something like MIN_TEMP_COUNT_DIFF and MAX_TEMP_COUNT_DIFF as 10, so you can change that "window" size of acceptable min temps easily.
TEMP_MIN will be 0, TEMP_MAX 1 and TEMP_AVG 2 -- which are the corresponding array indices. NUM_TEMPS will be 3, which is what you will use for your declaration (3 columns in the array) and for loops to process those columns.
Break your code into functions that are reusable. Consider something like
Code:
int get_min_temp(int monthly_temps[][NUM_TEMPS], int num_days)
{
int i, min_temp;
...
for (i = 0; i < num_days; i++)
// find min temp
return min_temp;
}
...
// in main
int min_temp = get_min_temp(temps, sizeof(temps) / sizeof(temps[0]));
Making functions like that make your code more maintainable and reusable. Imagine if you had two months worth of temps, and you wanted to compare their min, max and avg temps. You would either have to write the code twice and find/fix bugs twice, or write/debug it once, and simply call it (one line of code) twice.
The sizeof trick that I used ensures you always pass the right number of elements for the array. If I change temps to be NUM_DAYS, or NUM_HOURS (for hourly stats), I don't need to change that line.
Originally Posted by jimblumberg
