Are assignments in the condition part of conditionals a bad practice?

Question

Let's assume I want to write a function that concatenates two strings in C. The way I would write it is:

void concat(char s[], char t[]){
    int i = 0;
    int j = 0;

    while (s[i] != '\0'){
        i++;
    }

    while (t[j] != '\0'){
        s[i] = t[j];
        i++;
        j++;
    }

    s[i] = '\0';

}

However, K&R in their book implemented it differently, particularly including as much in the condition part of the while loop as possible:

void concat(char s[], char t[]){
    int i, j;
    i = j = 0;
    while (s[i] != '\0') i++;

    while ((s[i++]=t[j++]) != '\0');

}

Which way is preferred? Is it encouraged or discouraged to write code the way K&R do? I believe my version would be easier to read by other people.

Answer 1

Always prefer clarity over cleverness. In yesteryears the best programmer was that whose code nobody could understand. "I cannot make sense of his code, he must be a genius", they said. Nowadays the best programmer is that whose code anyone can understand. Computer time is cheaper now than programmer's time.

Any fool can write code that a computer can understand. Good programmers write code that humans can understand. (M. Fowler)

So, no doubt, I'd go for option A. And that is my definitive answer.

Answer 2

The golden rule, same as in Tulains Córdova's answer, is to make sure to write intelligible code. But I don't agree with the conclusion. That golden rule means to write code that the typical programmer who will end up maintaining your code can understand. And you are the best judge at who the typical programmer who will end up maintaining your code is.

For the programmers that didn't start with C, the first version is probably easier to understand, for reasons you know already.

For those who grew up with that style of C, the second version may well be easier to understand: to them, it's equally understandable what the code does, to them, it leaves fewer questions why it's written the way it is, and to them, less vertical space means that more context can be displayed on the screen.

You'll have to rely on your own good sense. To which audience do you want to make your code easiest to understand? Is this code written for a company? Then the target audience is probably the other programmers in that company. Is this a personal hobby project that no one will work on but yourself? Then you are your own target audience. Is this code you want to share with others? Then those others are your target audience. Pick the version that matches that audience. Unfortunately, there is no single preferred way to encourage.

Answer 3

EDIT: The line s[i] = '\0'; was added to the first version, thereby fixing it as described in variant 1 below, so this doesn't apply to the current version of the question's code anymore.

The second version has the distinctive advantage of being correct, while the first one is not - it does not null-terminate the target string correctly.

The "assignment in condition" allows expressing the concept of "copy every character before checking for the null character" very concisely and in a way that makes optimization for the compiler somewhat easier, though many software engineers these days find this style of code less readable. If you insist on using the first version, you'd have to either

1. add the null-termination after the end of the second loop (adding more code, but you can argue the readabiliy makes it worthwhile) or
2. change the loop body to "first assign, then either check or save the assigned char, then increment indices". Checking the condition in the middle of the loop means breaking out of the loop (reducing clarity, frowned upon by most purists). Saving the assigned char would mean introducing a temporary variable (reducing clarity and efficiency). Both of those would annihilate the advantage in my opinion.

Answer 4

The answers by Tulains Córdova and hvd cover the clarity/readability aspects quite well. Let me throw in scoping as another reason in favour of assignments in conditions. A variable declared in the condition is only available in that statement’s scope. You cannot use that variable afterwards by accident. The for loop has been doing this for ages. And it’s important enough that the upcoming C++17 introduces a similar syntax for if and switch:

if (int foo = bar(); foo > 42) {
    do_stuff();
}

foo = 23;   // compiler error: foo is not in scope

Answer 5

No. It is very standard and normal C style. Your example is a bad one, because it should just be a for loop, but in general there's nothing wrong with

if ((a = f()) != NULL)
    ...

for example (or with while).

Answer 6

Even being able to do this at all is a very bad idea. It's colloquially known as "The World's Last Bug," like so:

if (alert = CODE_RED)
{
   launch_nukes();
}

While you're not likely to make a mistake that's quite that severe, it's very easy to accidentally screw up and cause a hard-to-find error in your codebase. Most modern compilers will insert a warning for assignments inside a conditional. They're there for a reason, and you'd do well to heed them and just avoid this construct.

Answer 7

In the days of K&R

• ‘C’ was portable assembly code
• It was used by programmers that thought in assembly code
• The compiler did not do much optimization
• Most computers had “complex instruction sets”, for example while ((s[i++]=t[j++]) != '\0') would map to one instruction on most CPUs (I expect the Dec VAC)

There days

• Most people reading C code are not assembly code programmers
• C compilers do lots of optimization, hence the simpler to read code is likely to be translated into the same machine code.

(A note on always using braces – the 1st set of code takes up more space due to having some “unneeded” {}, in my experience these often prevent code that has been badly merged from compiler and allow errors with incorrect “;” placements to be detected by tools.)

However in the old days the 2nd version of the code would have read. (If I got it right!)

concat(char* s, char *t){      
    while (*s++);
    --s;
    while (*s++=*t++);
}

Answer 8

Both styles are well formed, correct, and appropriate. Which one is more appropriate depends largely upon your company's style guidelines. Modern IDEs will facilitate the use of both styles through the use of live syntax linting that explicitly highlight areas that might have otherwise become a source of confusion.

For example, the following expression is highlighted by Netbeans:

if($a = someFunction())

on the grounds of "accidental assignment".

To explicitly tell Netbeans that "yeah, I really meant to do that...", the expression can be wrapped in a set of parenthesis.

if(($a = someFunction()))

At the end of the day, it all boils down to company style guidelines and the availability of modern tools to facilitate the development process.