3
\$\begingroup\$

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)

\$\endgroup\$
  • \$\begingroup\$ // Print an error message — what error message? This code looks incomplete and not ready for review. \$\endgroup\$ – 200_success Aug 10 '17 at 5:56
  • \$\begingroup\$ @200_success - I've edited the question. Removed comment and added explanation about the error printing inside the functions. I've commented it out at first because I thought it is irrelevant for the question. \$\endgroup\$ – Gilad Green Aug 10 '17 at 5:58
  • \$\begingroup\$ Does that i + "" work in C? \$\endgroup\$ – 200_success Aug 10 '17 at 6:00
  • \$\begingroup\$ @200_success - haven't tested that line yet.. Will need to use sprintf instead. \$\endgroup\$ – Gilad Green Aug 10 '17 at 6:04
  • \$\begingroup\$ @200_success - The main point of the CR question is the use of the if statement that way. Not sure if it is considered a bad/ok/bad practice. And a bit less about the reading and classifying the lines. \$\endgroup\$ – Gilad Green Aug 10 '17 at 6:07
3
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Minor: "eliminate const from parameter m" --> leaving it in does serve a small purpose, (BTW, one I only use for large functions) in that it insures the function implementation does not later alter the value. Some programmers find greater understanding by seeing that - I usually see more clutter and prevention of code improvement you have. As part of a function declaration int foo(const int m); adds no value. \$\endgroup\$ – chux Aug 10 '17 at 14:34
  • \$\begingroup\$ Good idea about inverting the sense of stop to ok. (Avoids negation) and error message improvement. \$\endgroup\$ – chux Aug 10 '17 at 14:35
  • \$\begingroup\$ In the change from i < m && to m && , functionality has changed when m < 0. Recommend m > 0 && in your new loop or change to an unsigned type. \$\endgroup\$ – chux Aug 10 '17 at 14:37
  • 1
    \$\begingroup\$ @chux, yes, it seems to me that it should m should be of type size_t. \$\endgroup\$ – Edward Aug 10 '17 at 14:40
3
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer :) What I don't like about splitting the for this way (started from there..) is that there are 7-8 extra lines in code.. \$\endgroup\$ – Gilad Green Aug 10 '17 at 9:39
  • \$\begingroup\$ I don't see that as a problem, personally. My text editor and compiler can handle as many lines as I throw at them! 😉 But I know others who agree with you. I personally find code that's terse to be difficult to understand. One possible solution is to format your if statements differently. You could put the opening bracket on the same line as the if and put the first closing and second opening bracket on the same line as the else. Or leave the brackets off the else clause since it's one line. (I don't recommend that since it can lead to bugs, but it's an option.) \$\endgroup\$ – user1118321 Aug 10 '17 at 16:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.