My declarations and definitions of functions for my 2D array data are not working

Question

I have a data file containing 12 months of daily temperatures. I want the program to get the minimum and maximum temperature for each month using functions and printing in the main function. Somehow my function declarations and definitions are not accepted (following a tutorial I have). I tried to modify the program in the post here: Using a 2D Array in Function: Error, my program gets worse. Help needed. Here is the code

#include <stdio.h>
#include<stdlib.h>
  /* function declarations */
float max(float temp[][]);
float min(float temp[][]);

int main ()
{
    float maxt, temp[31][12];
    int i, j, year, month;
    char value, buff[1000];

    FILE *f1;
    FILE *f2;
    f1=fopen("temp_data.txt", "r");
    f2=fopen("temp_out.txt", "w");

    if (f1 == NULL)
    {
        printf("Can't open file\n",f1);
        return 1;
    }

    if (f2 == NULL)
    {
        printf("Can't open file\n",f2);
        return 1;
    }

    j=0;
    while (fgets(buff, sizeof(buff), f1))
    {
        sscanf(buff, " %i %i", &year, &month);
        for (i = 0; i <= 31; i++)
        {
          sscanf(buff, " %f", &temp[i][j]);
        }

        for(i = 0; i < 31; i++)
        {
            /* calling a function to get max value */
            float maxt = max(temp);
            float mint = min(temp);
            printf( " %d Min=%5.1f Max=%5.1f\n", j, mint, maxt);
        }
        j=j+1
    }
    return 0;
}

/* function returning the max of 31 temperatures */
float max(float temp[][])
{
 /* local variable declaration */
 float maxt;
 maxt=-99.;
 int i,j;
 for(i = 0; i < 31; i++)
 {
  if(temp[i][j] > maxt)
  {
     maxt=temp[i][j];
  }
 }
 return maxt;
 }

/* function returning the max of 31 temperatures */
float min(float temp[][])
{
 /* local variable declaration */
 float mint;
 mint=99.;
 int i,j;
 for(i = 0; i < 31; i++)
 {
  if(temp[i][j] < mint)
  {
     mint=temp[i][j];
  }
 }
 return mint;
 }

Here is my data

 2012  01    29.6    32.2    32.5    32.7    31.0    28.0    29.6    30.0    32.0    30.5    32.0    27.5    27.8    30.0    30.5    28.6    30.5    29.0    29.7    30.0    32.2    31.6    29.5    31.0    31.5    30.0    29.5    32.0    32.3    32.5    27.7
 2012  02    32.2    30.5    32.0    32.6    30.0    32.4    33.0    29.8    32.0    33.0    30.2    29.0    29.2    31.0    27.5    31.0    28.0    25.0    25.8    28.5    32.3    31.0    29.0    31.3    30.5    28.1    30.0    29.2    26.3   999.9   999.9
 2012  03    23.5    30.5    30.8    30.2    29.7    30.0    29.0    28.3    31.8    31.3    31.6    31.4    32.0    33.5    31.6    33.0    33.4    32.7    30.6    32.5    30.5    33.0    32.5    30.0    23.7    28.3    29.5    32.0    33.6    33.5    29.5
 2012  04    29.8    30.5    30.3    32.1    27.5    29.0    29.6    29.7    30.5    32.6    31.8    30.5    30.1    29.7    24.5    25.6    29.6    30.2    30.0    30.4    30.0    31.1    31.2    28.4    30.2    31.2    30.0    30.6    31.8    28.3   999.9
 2012  05    30.2    28.4    30.7    26.8    27.6    29.8    29.0    27.5    30.0    27.0    27.2    30.5    29.5    28.7    29.8    29.5    30.7    30.8    30.5    31.5    29.0    30.6    31.6    30.5    30.7    31.0    30.8    29.5    29.0    30.6    31.0
 2012  06    30.8    30.2    30.5    28.0    29.8    30.3    30.3    31.0    31.3    32.2    31.0    30.3    30.4    30.8    31.0    29.5    30.0    30.1    30.0    30.5    30.5    31.0    31.2    31.5    31.1    26.9    25.3    30.0    30.7    29.2   999.9
 2012  07    28.8    31.3    32.0    31.5    30.7    30.9    30.5    31.3    31.5    32.6    31.3    31.1    31.2    31.0    30.5    32.0    32.5    30.7    30.2    30.4    31.0    31.4    31.6    30.6    30.7    30.0    28.8    29.1    30.8    31.0    30.2
 2012  08    32.0    31.6    32.4    32.2    32.5    32.7    33.8    27.5    29.5    31.6    31.7    32.0    32.3    33.0    31.8    33.4    34.0    32.8    33.0    32.4    33.0    33.2    34.1    34.3    34.6    34.8    28.0    32.2    32.5    27.5    31.8
 2012  09    33.0    30.4    29.5    31.6    31.8    30.2    31.0    32.0    29.0    25.0    31.4    28.8    29.0    30.6    30.3    31.0    32.2    30.0    31.0    32.1    33.5    33.2    34.0    33.0    32.4    31.0    31.2    32.4    32.8    33.0   999.9
 2012  10    30.5    31.5    32.5    32.3    33.1    32.0    31.7    31.5    32.2    30.3    31.0    28.2    31.0    30.8    33.0    31.3    31.8    31.5    28.0    30.6    31.2    30.6    31.8    31.4    25.0    30.8    30.3    31.0    32.0    31.6    29.4
 2012  11    29.0    28.0    29.8    28.6    26.7    29.5    25.7    31.5    27.0    29.4    30.0    32.7    32.5    30.7    29.5    29.6    29.0    30.0    29.0    28.5    28.8    29.6    28.1    28.8    28.5    29.3    30.7    28.0    29.5    30.3   999.9
 2012  12    30.8    32.6    28.6    31.3    23.7    27.3    27.0    28.5    32.1    29.5    28.7    31.0    27.3    30.1    27.8    29.2    29.5    30.0    29.3    32.4    30.0    29.3    29.5    30.1    29.4    28.8    30.4    29.5    29.2    27.3    29.3

errors in function declarations: "array type has incomplete element type" errors in function definitions: "type of forma parameter 1 is incomplete" "two few arguments to function 'min', 'max'" and "conflicting types for 'min', 'max'" also 'j' undeclared here (not in a function

Help will be appreciated.

This is a candidate for the most horrible abuse of the `sscanf` and format strings I have ever seen. — Eugene Sh., Aug 01 '17 at 21:58
so you can correct me, I said am getting this from a tutorial. — Zilore Mumba, Aug 01 '17 at 22:00
The `fgets` line should be `while (fgets(buff, sizeof(buff), f1))` and the `sscanf` line should be something like `sscanf(buff, " %i %i", &year, &month); for (i = 0; i <= 31; i++) sscanf(buff, " %f", &temp[i][j]);` instead of...that. Both more readable and handles whitespace better. And you *might* want to find a different tutorial. The rest of that code isn't *too* bad, but that sscanf line... — Ray, Aug 01 '17 at 22:10
As for the actual problem, could you post a couple lines of the data file and the error/results you're getting? — Ray, Aug 01 '17 at 22:14
See [Using `sscanf()` in loops](http://stackoverflow.com/questions/3975236/how-to-use-sscanf-in-loops) for a discussion of how to use `sscanf()` in a loop. What do you do with February's data, when there are at most 29 and usually only 28 entries — assuming a full month's data? You should check the return value from `sscanf()`. — Jonathan Leffler, Aug 01 '17 at 22:32
the posted code does not compile! there compiler outputs several warning message and an error message on this function signature; `float min(float temp[][])` — user3629249, Aug 01 '17 at 22:34
For ease of readability and understanding, 1) Consistently format the code. You could select your code then click the '{}' icon/button 2) follow the axiom: *only one statement per line and (at most) one variable declaration per statement.* — user3629249, Aug 01 '17 at 22:37
"am not able to see how to attach my data file" --> post a few lines of it then. — chux - Reinstate Monica, Aug 01 '17 at 22:38
regarding these two lines: `float maxt; maxt=-99.;` the variable `maxt` is declared as a `float`, but the literal `-99,` is a `double`. Suggest replacing both lines with: `float maxt = -99.0f;` — user3629249, Aug 01 '17 at 22:39
in the function: `max()`, the variable 'j' is being used without having been initialized to a known value. Such usage of a uninitialized variable is undefined behavior and can lead to a seg fault event. The same problem is shown in the `min()` function with its' 'j' variable. — user3629249, Aug 01 '17 at 22:41
the posted code contains some random 'back ticks' in column 1, for instance on the line: "` if (f2 == NULL)" and on the line: "` return 0;" — user3629249, Aug 01 '17 at 22:46
Note that: `float max(float temp[][]); float min(float temp[][]);` is not acceptable C. You can only omit the first dimension of the array; the others must be specified somehow. Possibly as part of a VLA, possibly as a fixed size. — Jonathan Leffler, Aug 01 '17 at 22:50
Note that the best way to determine an initial value for the min/max in the functions is to save the element in `array[0]` as the initial guess, and then test the other entries against that: `float mint = temp[0]; for (int i = 1; i < 31; i++) { if (temp[i] < mint) mint = temp[i]; }`. It's also unclear why you're passing a 2D array to the function since you never initialize `j`, let alone increment it. — Jonathan Leffler, Aug 01 '17 at 22:54
Come on, man. You want others to read and assist. Make an effort to format your post, code and data, so it's readable. — Bjorn A., Aug 02 '17 at 16:32

Jonathan Leffler · Accepted Answer · 2017-08-02T19:46:38.377

Critique based on Revision 3

There are numerous problems — I may not have recorded and reported them all in the list below.

You open an output file and never use it. (Code removed.)
You open a fixed-name input file — don't. Read from standard input (as in the code below), or take a file name on the command line an open that. Fixed names are unnecessarily restrictive.
You use %i to read integers. That's good if you aren't reading decimal numbers with leading zeros. However, when sscanf() sees 08 with %i, it reads that as 0 because 8 is not a valid octal number.
You didn't lay out the sscanf() code systemtically; that makes it unreadable, and unmaintainable.
Your data uses 999.9 to indicate 'non-existent date'. That's one way to do it, but it complicates the code enormously.
You use your array cack-handed (and I'm speaking as a leftie myself). You should have 12 rows (one per month) of 31 days, not 31 rows (one per day) of 12 months. Hence temp[12][31] in the code.
Doing that means you can pass one month's worth of data to the aggregate functions without having to futz with 2D array notations in the functions.
Your function declarations wouldn't compile because you needed max(float temp[][12]); in your scheme. You must provide the sizes of all array dimensions except the first.
The replacement code below gets a single month's worth of data by passing temp[month] to the functions.
It would be possible to simply omit the non-existent days (31st September; 30th and 31st February — since the year in the data was 2012, a leap year, February had 29 days that year). When I was generating myself some test data (before you added yours), the same code was working happily with 28 entries for February, 30 for September, April, June and November, and 31 for the rest. (That cuts down on the complexity of handling 999.9.)
It might be sensible to call cnt() first, and then pass that count to the other aggregate functions. It would allow you to omit the dummy values more easily. You could also verify that the dummy values are only at the end of each line.
For determining the minimum and maximum, the code uses the first temperature (and asserts that it is not a dummy temperature) as the initial guess, and then checks the remaining temperatures against that. This is usually the best way to deal with the 'initial value' problem. It fails if a count of zero is permitted — the code should (but doesn't) assert against that.
I added avg() to calculate the average for interest's sake.
I added cnt() because it was necessary to discount the the dummy entries.

The question morphed

The version of the code I started from (Revision 3 in the history) contained the statements:

j=0;
while (fgets(buff, 300, f1) != NULL)
{
sscanf(buff,"%i%i%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f",
       &year,&month,&temp[0][j],&temp[1][j],&temp[2][j],&temp[3][j],&temp[4][j],&temp[5][j],&temp[6][j],&temp[7][j],&temp[8][j],&temp[9][j],&temp[10][j],
       &temp[11][j],&temp[12][j],&temp[13][j],&temp[14][j],&temp[15][j],&temp[16][j],&temp[17][j],&temp[18][j],&temp[19][j],&temp[20][j],&temp[21][j],
       &temp[22][j],&temp[23][j],&temp[24][j],&temp[25][j],&temp[26][j],&temp[27][j],&temp[28][j],&temp[29][j],&temp[30][j],&temp[31][j]);

for(i = 0; i < 31; i++)
{
/* calling a function to get max value */
float maxt = max(temp);
float mint = min(temp);
printf( " %d Min=%5.1f Max=%5.1f\n", j, mint, maxt);
}
j=j+1
}

I didn't notice that the code in the question had been rewritten since I took the snapshot.

Additional issues:

The sscanf() statement has a reference to &temp[31][j] which doesn't exist as part of the array. There isn't a conversion specification (%f) for it anyway.
This is a messy way to write the code, but I retained it.
The loops after the sscanf() are confusing, or confused. Iterating 31 times to find the maximum is not appropriate; ditto the minimum. There is a simple disconnect between requirements and implementation here.

Critique of later revisions

The attempted rewrite using sscanf() iteratively doesn't work. To see what's required to make it work, consult Using sscanf() in loops. The %n conversion specification is crucial to getting it to work sanely.
My code preserves the original rather than trying to do the scan-in-a-loop logic.

Revised code

Here's the revised code — based on the Revision 3 outline:

/* SO 4544-8234 Daily temperatures */
#include <assert.h>
#include <stdio.h>
#include <stdlib.h>

#define MIN_VALID_TEMP -100.0
#define MAX_VALID_TEMP +300.0

float max(int ndays, float temp[]);
float min(int ndays, float temp[]);
float avg(int ndays, float temp[]);
int   cnt(int ndays, float temp[]);

int main(void)
{
    float temp[12][31];
    char buff[1000];

    for (int j = 0; fgets(buff, 300, stdin) != NULL; j++)
    {
        int year, month;
        int nf = sscanf(buff, "%d%d%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f%f",
                   &year, &month,
                   &temp[j][ 0], &temp[j][ 1], &temp[j][ 2], &temp[j][ 3],
                   &temp[j][ 4], &temp[j][ 5], &temp[j][ 6], &temp[j][ 7],
                   &temp[j][ 8], &temp[j][ 9], &temp[j][10], &temp[j][11],
                   &temp[j][12], &temp[j][13], &temp[j][14], &temp[j][15],
                   &temp[j][16], &temp[j][17], &temp[j][18], &temp[j][19],
                   &temp[j][20], &temp[j][21], &temp[j][22], &temp[j][23],
                   &temp[j][24], &temp[j][25], &temp[j][26], &temp[j][27],
                   &temp[j][28], &temp[j][29], &temp[j][30]);

        assert(nf >= 28 + 2 && nf <= 31 + 2);   // 28-31 days, plus year plus month
        int ndays = nf - 2;
        float maxt = max(ndays, temp[j]);
        float mint = min(ndays, temp[j]);
        float avgt = avg(ndays, temp[j]);
        int count  = cnt(ndays, temp[j]);
        printf("%.4d-%.2d: days = %d, min = %6.2f, avg = %6.2f, max = %6.2f\n", year, month, count, mint, avgt, maxt);
    }
    return 0;
}

float max(int ndays, float temp[])
{
    float maxt = temp[0];
    assert(maxt > MIN_VALID_TEMP && maxt < MAX_VALID_TEMP);
    for (int i = 1; i < ndays; i++)
    {
        if (temp[i] > MIN_VALID_TEMP && temp[i] < MAX_VALID_TEMP)
        {
            if (temp[i] > maxt)
                maxt = temp[i];
        }
    }
    return maxt;
}

float min(int ndays, float temp[])
{
    float mint = temp[0];
    assert(mint > MIN_VALID_TEMP && mint < MAX_VALID_TEMP);
    for (int i = 1; i < ndays; i++)
    {
        if (temp[i] > MIN_VALID_TEMP && temp[i] < MAX_VALID_TEMP)
        {
            if (temp[i] < mint)
                mint = temp[i];
        }
    }
    return mint;
}

float avg(int ndays, float temp[])
{
    float sum = 0;
    int count = 0;
    for (int i = 0; i < ndays; i++)
    {
        if (temp[i] > MIN_VALID_TEMP && temp[i] < MAX_VALID_TEMP)
        {
            count++;
            sum += temp[i];
        }
    }
    return sum / ndays;
}

int cnt(int ndays, float temp[])
{
    int count = 0;
    for (int i = 0; i < ndays; i++)
    {
        if (temp[i] > MIN_VALID_TEMP && temp[i] < MAX_VALID_TEMP)
            count++;
    }
    return count;
}

On your sample data, the output I get is:

2012-01: days = 31, min =  27.50, avg =  30.44, max =  32.70
2012-02: days = 29, min =  25.00, avg =  28.08, max =  33.00
2012-03: days = 31, min =  23.50, avg =  30.76, max =  33.60
2012-04: days = 30, min =  24.50, avg =  28.93, max =  32.60
2012-05: days = 31, min =  26.80, avg =  29.68, max =  31.60
2012-06: days = 30, min =  25.30, avg =  29.21, max =  32.20
2012-07: days = 31, min =  28.80, avg =  30.88, max =  32.60
2012-08: days = 31, min =  27.50, avg =  32.20, max =  34.80
2012-09: days = 30, min =  25.00, avg =  30.21, max =  34.00
2012-10: days = 31, min =  25.00, avg =  30.96, max =  33.10
2012-11: days = 30, min =  25.70, avg =  28.33, max =  32.70
2012-12: days = 31, min =  23.70, avg =  29.34, max =  32.60

Nobody pointed out the OP's fallacy of trying to read data out of a character array with sscanf the same way as of a FILE* with fscanf. Unfortunately, a string does not keep track of a reading position (that's actually an idea...), so all the sscanf read from the string's beginning. They probably (almost) all succeed, since one can read a float from a valid int string, and often vice versa. With all due respect I must say that I find your solution to read 33 values at once ... abhorrent. I would either demand C11 with string literal concatenation, or build the format string dynamically. — Peter - Reinstate Monica, Aug 02 '17 at 19:28
Building the format string dynamically has the advantage of catering to the different month lengths because year (leap years!) and month are known at run time at that point.-- Of course otherwise a good answer with much too much work put into it... — Peter - Reinstate Monica, Aug 02 '17 at 19:30
The 33-at-once is what the OP had in their code — and apart from trying to pass a non-existent element of the array to `sscanf()` when there wasn't a format letter for it (a detail I forget to note before) — it was 'OK'. At least the code in the answer checks for plausibility from the return value of `sscanf()` rather than blithely assuming. What's not so obvious was that the `%i` to `%d` change was only spotted because August's minimum was 8 and September's 9, which didn't seem plausible. There are other changes that can be made to the code; it isn't a perfect solution. — Jonathan Leffler, Aug 02 '17 at 19:33
Agree on 'much too much work' part. But I started on it because it was a chance to write some data generating scripts that I'd been meaning to get around to writing at some point. They're not perfect, and not germane to the answer now that the OP has provided the sample data, but the whole was an excuse to do something I'd been thinking about doing for N years (where N exceeds 1 by a considerable margin). — Jonathan Leffler, Aug 02 '17 at 19:35
@PeterA.Schneider: Hmm; your comments about `sscanf()` in a loop are on target — I just hadn't noticed that the OP had modified the code in the question. I've now identified what I was working from — **Thank You** for pointing out the issue. I guess that if I'd not seen the original, I'd have written a different program, using `sscanf()` in a loop correctly. I'm not sure I've got the energy left to add that alternative — I'll leave it as an exercise for the OP. At least the code was using the `fgets()` plus `sscanf()` combination; that is crucial to sane operation. — Jonathan Leffler, Aug 02 '17 at 19:48
my very sincere thanks @Jonathan Leffler for the time and effort to provide a very comprehensive critique of my program and for providing a working version. I am able to compile and link the program, but the exe is not giving me any output. In time I will sort that out. (I am working in Windows 10) with codeblocks. — Zilore Mumba, Aug 02 '17 at 20:50
Most likely, you are expecting it to read from your file where this code reads from standard input. You'd need to run: `./program < temp_data.txt` to feed it the information. If you prefer (but you don't), you could type the data at the command line. At least, typing `1999 12 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1` on a line would show the program was reading input. Or you can reinsert your file opening code and change the `fgets()` to read from the file you open. — Jonathan Leffler, Aug 02 '17 at 20:52
@Jonathan Re "At least the code was using the fgets() plus sscanf() combination; that is crucial to sane operation": Well... because of the trouble with sscanf reading positions I prefer reading from files directly and then obviously like data which does not rely on lines because, to me and to scanf, all whitespace was created equal ;-). The OP's "record end marker value" of 999.9 idea isn't that bad but should be used for every line, and it would be nice to have a better value (but which? probably something non-numeric so that a numeric conversion fails). — Peter - Reinstate Monica, Aug 02 '17 at 20:55
Btw, has anybody ever written a "string device" for a *nix? That should be a no-brainer (open, read, write, close, seek are all trivial). Much weirder things have bent to fit the "everything is a file" paradigm. That little wrapping and book keeping would make strings really look like any other input. — Peter - Reinstate Monica, Aug 02 '17 at 21:04
Exactly, I did not realise the program was waiting for data from stdin. When I introduced the data,it works perfectly. Once more thanks very much, it is a fantastic program — Zilore Mumba, Aug 03 '17 at 05:21

My declarations and definitions of functions for my 2D array data are not working

1 Answers1

Critique based on Revision 3

The question morphed

Critique of later revisions

Revised code