“Look what I can do!” – Unacceptable in programming

Now we are going to touch upon a simple 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 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, we 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

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

This tip sounds trivial, but we 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.

Written by Andrey Karpov.

This error was found with PVS-Studio static analysis tool.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s