34

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.

  • 38
    Don't forget, K&R was first published in 1978. There's been a couple small changes in how we code since then. – corsiKa Oct 29 '16 at 23:15
  • 27
    Readability was very different back in the days of teleprinters and line-oriented editors. Mushing all that stuff onto a single line used to be more readable. – user2357112 Oct 30 '16 at 1:19
  • 15
    I'm shocked that they have indices and comparisons to '\0' instead of something like while (*s++ = *t++); (My C is very rusty, do I need parens there for operator precedence?) Did K&R release a new version of their book? Their original book had extremely concise and idiomatic code. – user949300 Oct 30 '16 at 5:28
  • 4
    Readability is a very personal thing - even in the days of teletypes. Different people prefer different styles. A lot of the instruction cramming had to do with the code generation. In those days, some instructions sets (eg Data General) could cram several operations into one instruction. Also, in the early 80s, there was a myth that using parenthesis generated more instructions. I had to generate the assembler to prove to the code reviewer that it was a myth. – cup Oct 30 '16 at 10:54
  • 10
    Note that the two code blocks are not equivalent. The first code block will not copy the terminating '\0' from t (the while exits first). This will leave the resulting s string without a terminating '\0' (unless the memory location was already zeroed). The second code block will make the copy of the terminating '\0' prior to exiting the while loop. – Makyen Oct 30 '16 at 12:08
79

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 doudbt, I'd go for option A. And that is my definitive answer.

  • 8
    Very pithy ideology, but the fact remains that there's nothing wrong with assignment in conditionals. It's far preferable to early exit from a loop or duplicating code before and inside a loop. – Miles Rout Oct 29 '16 at 20:31
  • 25
    @MilesRout There is. There is something wrong with any code having a side effect where you don't expect it, i.e. passing function arguments or evaluating conditionals. Not even mentionning that if (a=b) can easily be mistaken for if (a==b). – Arthur Havlicek Oct 29 '16 at 21:06
  • 12
    @Luke: "My IDE can clean up X, therefore it's not a problem" is rather unconvincing. If it's not a problem, why does the IDE make it so easy to "fix?" – Kevin Oct 30 '16 at 5:32
  • 6
    @ArthurHavlicek I agree with your general point, but code with side effects in conditionals is really not so uncommon: while ((c = fgetc(file)) != EOF) as the first that comes to my mind. – Daniel Jour Oct 30 '16 at 5:59
  • 3
    +1 "Considering that debugging is twice as hard as writing a program in the first place, if you're as clever as you can be when you write it, how will you ever debug it?" B.W.Kernighan – Christophe Oct 31 '16 at 20:24
32

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.

14

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.
  • Correct is better than readable and concise. – user949300 Oct 31 '16 at 20:01
5

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
3

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).

  • 7
    There is something wrong with it; ` != NULL` and its kin in a C conditional are natter, just there to placate developers who aren't comfortable with the concept of a value being true or false (or vice-versa). – Jonathan Cast Oct 30 '16 at 15:33
  • 1
    @jcast No, it's more explicit to include != NULL. – Miles Rout Oct 30 '16 at 21:35
  • 1
    No, it's more explicit to say (x != NULL) != 0. After all, that's what C is really checking, right? – Jonathan Cast Oct 30 '16 at 21:38
  • @jcast No, it isn't. Checking whether something is unequal to false is not how you write conditionals in any language. – Miles Rout Oct 30 '16 at 21:41
  • "Checking whether something is unequal to false is not how you write conditionals in any language." Exactly. – Jonathan Cast Oct 30 '16 at 21:43
2

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++);
}
2

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.

  • Before these warning, we would write CODE_RED = alert so it gave a compiler error. – Ian Oct 31 '16 at 21:12
  • 4
    @Ian Yoda Conditionals that is called. Hard to read they are. Unfortunate the necessity for them is. – Mason Wheeler Oct 31 '16 at 21:14
  • After a very brief introductory "getting used to it" period, Yoda conditions are no harder to read than normal ones. Sometimes they are more readable. For example, if you have a sequence of ifs / elseifs, having the condition being tested against on the left for higher emphasis is a slight improvement IMO. – user949300 Nov 1 '16 at 0:38
  • 2
    @user949300 Two words: Stockholm Syndrome :P – Mason Wheeler Nov 1 '16 at 1:36
2

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".

enter image description here

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()))

enter image description here

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.

protected by gnat Oct 30 '16 at 19:50

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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