16. "Look what I can do!" - Unacceptable in programming

This section will be slightly similar to "Don't try to squeeze as many operations as possible in one line", but this time I want to focus on a different thing. Sometimes it feels like programmers are competing against somebody, trying to write the shortest code possible.

I am not speaking about complicated templates. This is a different topic for discussion, as it is very hard to draw a line between where these templates do harm, and where they do good. Now I am going to touch upon a simpler situation which is relevant for both C and C++ programmers. They tend to make the constructions more complicated, thinking, "I do it because I can".

The fragment is taken from KDE4 project. The error is detected by the following PVS-Studio diagnostic: V593 Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'.

void LDAPProtocol::del( const KUrl &_url, bool )
{
  ....
  if ( (id = mOp.del( usrc.dn() ) == -1) ) {
    LDAPErr();
    return;
  }
  ret = mOp.waitForResult( id, -1 );
  ....
}

Explanation

After looking at this code, I always have questions such as: What was the point of doing it? Did you want to save a line? Did you want to show that you can combine several actions in one expression?

As a result we have a typical error pattern - using expressions of the if (A = Foo() == Error) kind.

The precedence of the comparison operation is higher than that of the assignment operation. That's why the "mOp.del( usrc.dn() ) == -1" comparison is executed first, and only then the true (1) or false (0) value is assigned to the id variable.

If mOp.del() returns '-1', the function will terminate; otherwise, it will keep running and the 'id' variable will be assigned an incorrect value. So it will always equal 0.

Correct code

I want to emphasize: adding extra parentheses is not a solution to the problem. Yes, the error can be eliminated. But it's the wrong way.

There were additional parentheses in the code - have a closer look. It's difficult to say what they were meant for; perhaps the programmer wanted to get rid of the compiler warnings. Perhaps he suspected that the operation priority may be not right, and wanted to fix this issue, but failed to do so. Anyway, those extra brackets don't help.

There is a deeper problem here. If it is a possible not to make the code more complicated, don't. It is better to write:

id = mOp.del(usrc.dn());
if ( id == -1 ) {

Recommendation

Don't be so lazy as not to write an extra code line: complex expressions are hard to read, after all. Do the assignment first, and only then, the comparison. Thus you will make it easier for programmers who will be maintaining your code later, and also it will reduce the chances of making a mistake.

So my conclusion is - don't try to show off.

This tip sounds trivial, but I hope it will help you. It's always better to write clear and neat code, instead of in a "see how cool I am" style.