Reading and classifying lines from a file

Question

I'm new to C which I'm learning in university now, and I'm not sure if the following is considered in C good practices or not.

For an assignment in a simple Classification problem I wrote the following code:

int train(FILE *file, const dim dim, const int m, vector *linearSeparator)
{
    size_t len = 0;
    char *line;
    bool stop = false;

    Sample *sample = NULL;
    for(int i = 0; i < m && getline(&line, &len, file) != -1 && !stop; i++)
    {
        if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
            calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
        {
            printError(TRAINING, "Failed processing line");
            stop = true;
        }
        free(line);
        freeSample(sample);
    }
    line = NULL;
    return stop ? EXIT_FAILURE : EXIT_SUCCESS;
}

The functions:

1. strToSample gets a char* line and constructs a Sample object out of it, setting the *sample pointer. It returns EXIT_SUCCESS if succeeded and an error code if failed.
2. calibrateLinearSeparator uses the sample set by the previous function to manipulate the linearSeparator object (vector - a custom struct to represent a double[]). This function too returns EXIT_SUCCESS if succeeded and an error code if failed.

Both these functions print details about their internal errors and at this level I'm printing in what sample id that internal error occurred.

My question is:

1. Is this way of writing the if statement a good practice? Is it readable? Should I do it differently?

   It feels as if these functions are having side effects. They get pointers to set but are used in an if statement to check if they succeeded in a way that order matters.

   The idea behind writing it this way is that if either of these functions fail I want to print an error and not continue the loop. If the first one fails then the second is not performed (which is what I want). In any case of success or failure I want to free the allocated memory.

2. Is it a good practice in C to write the for condition the way I did? Having 3 conditions in it? (checking i, trying to bring next line and !stop?) It seems to me that this helps making the code cleaner as there are less "details" lines (of increasing i++ for example if the loop was a while loop on getLine)

Answer 1

The review from @chux covers most of what I'd have said, so I'll just add a few additional points.

Avoid variable type and name clashes

The function prototype has const dim dim as a parameter which is very peculiar. The only way this compiles if if dim is a user-defined type and also the parameter name. That's a recipe for confusion and should be avoided.

Understand the meaning of const

When a formal parameter is labeled const as with dim and m in this function, it means they can't be altered. For complex types and pointers this is a good idea, but if we're passing an int, it probably doesn't make much difference. The reason is that plain types like int and char are passed by value, so the function only has a copy of the variable anyway. This means that one could, in this case, eliminate const from parameter m (and make it of type size_t -- negative numbers don't seem to have any plausible semantic meaning here) and then we no longer really need variable i because the for loop can be written like this:

for ( ; m && !stop && getline(&line, &len, file) != -1; --m)

In this case, I've slightly rearranged the terms to make sure we only read a line when both m is nonzero and stop is false.

Set a boolean variable directly

Instead of testing a condition and then explicitly setting a boolean based on that, I'd suggest simply setting the boolean value directly. I'd name the variable ok inverting its sense, and write the loop like this:

for ( ; m && ok && getline(&line, &len, file) != -1; --m)
{
    ok = strToSample(line, SEPARATOR, dim, sample) == EXIT_SUCCESS &&
        calibrateLinearSeparator(sample, linearSeparator, dim) == EXIT_SUCCESS;
    freeSample(sample);
}

Then cleanup and error handling is done after the loop:

free(line);
if (!ok) {
    printError(TRAINING, "Failed processing line");
    return EXIT_FAILURE;
}
return EXIT_SUCCESS;

Alternatively, if you want to give the user a little hint as to where the error might be, reintroduce line counter i and alter the printError code within the loop to be something like this:

printError(TRAINING, "Failed processing line %d", i+1);

Use the most appropriate return value type

It appears that train only returns EXIT_SUCCESS or EXIT_FAILURE. That strongly suggests to me that its return type should be bool and that it should return true or false instead. If that's done, instead of this:

return stop ? EXIT_FAILURE : EXIT_SUCCESS;

One could write this:

return stop;

I'd prefer the shorter, simpler version. The same thing probably applies to strToSample and calibrateLinearSeparator as well.

Answer 2

Code has bug. After free'ing line, set len = 0; line = NULL;

    free(line);
    line = NULL; // add
    len = 0; // add

Further, it would be better to initialize line and move this free(); ... to outside the for() loop. Also line = NULL; len = 0; would not be needed there.

size_t len = 0;
char *line = NULL;
...
for(...; ... getline(&line, &len, file) ...; ...) {
  ...
}
free(line);

---

Is this way of writing the if statement a good practice? Is it readable? Should I do it differently?

It is not clear that the { printError(... true; } block is part of if() and not a free standing block. Consider the below. The first lacks clarity that the first block is part of the if() and the 2nd block simply a stand-along block

    if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
        calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
    {
        printError(TRAINING, "Failed processing line");
        stop = true;
    }
    {
        printError(TRAINING, "Status");
    }
    free(line);
    freeSample(sample);

vs. this form. The 1st block is part of the if() and the 2nd block is not nested like the first.

    if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
        calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
      {
        printError(TRAINING, "Failed processing line");
        stop = true;
      }
    {
        printError(TRAINING, "Status");
    }
    free(line);
    freeSample(sample);

---

Is it a good practice in C to write the for condition the way I did?

Why read a line if stop is true? Certainly this is surprising functionality. Given 3 stopping conditions, it appears to me that the combination of them occurring in groups, the side effects of getline() , and the short-circuit nature of && led to weak function flow.

for(int i = 0; i < m && getline(&line, &len, file) != -1 && !stop; i++)
// vs. 
for(int i = 0; i < m && !stop && getline(&line, &len, file) != -1; i++)

IMO, a for() loop is best as for (initialize_index, index_test; index_delta). Another other stopping condition belongs in the body. Example:

for (int i = 0; i < m; i++) {
  if (stop || getline(&line, &len, file) == -1) {
    break;
  }
  ...
}

Notice this form avoid ! in 2 places. Negation adds to complexity. Don't you not think it is not so and not so when not done?

Answer 3

Is this way of writing the if statement a good practice? Is it readable? Should I do it differently?

In my opinion (and others will have other opinions) this is fine. You have 2 conditions which is reasonable to write inline. You've formatted them in a readable way, so it's pretty clear what's going on. There are side effects, but that's normal in imperative programming. If you were to have more conditions, especially ones where it wasn't obvious how they fit together, I would recommend making a single function that returns true or false based on the conditions and name it something appropriate to convey that it's testing all the conditions that need to be tested.

Is it a good practice in C to write the for condition the way I did? Having 3 conditions in it?

This one's a little trickier. I find it a little harder to understand. Personally, I would write it as:

Sample *sample = NULL;
for(int i = 0; i < m && !stop; i++)
{
    if (getline(&line, &len, file) != -1)
    {
        if (strToSample(line, SEPARATOR, dim, sample) != EXIT_SUCCESS ||
            calibrateLinearSeparator(sample, linearSeparator, dim) != EXIT_SUCCESS)
        {
            printError(TRAINING, "Failed processing line");
            stop = true;
        }
        free(line);
        freeSample(sample);
    }
    else
    {
        stop = true;
    }
}

However, I know many other programmers who would write it as you did, so take that for what it's worth.