6
\$\begingroup\$

For a while now, our team has been doing peer code reviews of all submissions. As we've learned, everyone has different styles and going forward, all of us see a need to converge at some kind of tribal/team style so that the code looks consistent and we don't spend time in code reviews arguing one style vs. another.

For most part we've been able to come up with compromises but few, such as this one, are turning out to be sticky:

if ((bIsPaused) || (runState != LnrState_Running))
{ 
    ....
}

vs.

if (bIsPaused || runState != LnrState_Running)
{
    ....
}

One one side, the argument is less characters -> less information to process -> more readable code.

On the other side, the argument is that extra parenthesis help to read the code.

I'm curious if this is a purely personal thing and the entire community split 50/50 on this (in which case, I'm probably going to be bashed for asking this question), or if you guys think one way is actually more preferred than the other.

UPDATE: removed spaces in the example code so the answers/comments just focus on existence/absence of extra parenthesis pairs.

\$\endgroup\$
10
  • 4
    \$\begingroup\$ This is a waste of time. There is being consistent then there is micro management. As long as it is readable and correct. Legislating every single thing is silly and counterproductive. \$\endgroup\$ – Martin York Feb 24 '12 at 18:35
  • \$\begingroup\$ @LokiAstari: sure, but you can also argue that agreeing on spacing style is also micro management, but every single coding style document tells you exactly where to put whitespace. \$\endgroup\$ – DXM Feb 24 '12 at 18:41
  • 2
    \$\begingroup\$ @DMX: No they do not. Only bad ones. The coding standard (for this situation) should read "Make it readable". That is all. This is just sheer stupidity brought on by "I have seen a coding standard now I want to write one". The standard is supposed to make it easier to read/write not make pointless rules. \$\endgroup\$ – Martin York Feb 24 '12 at 19:03
  • 1
    \$\begingroup\$ @DMX: No they do not. Only bad ones. Infact the extra spaces around your '(' and ')' make it less readable in my opinion as you are deviating from the normal English writing conventions (which is what people are used to (assuming an English speaking team)). The coding standard (for this situation) should read "Make it readable". That is all. This is just sheer stupidity brought on by "I have seen a coding standard now I want to write one". The standard is supposed to make it easier to read/write not make pointless rules. \$\endgroup\$ – Martin York Feb 24 '12 at 19:13
  • 2
    \$\begingroup\$ i would skip all those extra space bars, between parenthesis etc. \$\endgroup\$ – john-jones Feb 24 '12 at 20:24
11
\$\begingroup\$

Compare this:

if ( ( bIsPaused ) || ( runState != LnrState_Running ) )

And this:

if ( [ bIsPaused ] || [ runState != LnrState_Running ] )

You'll find, I think, that the second is much easier to read. This is because I'm using multiple types of braces (illegal, but bear me with here). As humans we can identify the different symbols very quickly, but we can't really line up matching symbols all that quickly.

This technique:

if ( ( bIsPaused ) || ( runState != LnrState_Running ) )

Depends on pairing up symbols, something we aren't actually all that good at.

However, this technique:

if ( bIsPaused || runState != LnrState_Running )

Depends on our ability to distinguish symbols, which is really good, and not on pairing symbols. It does also depend on our ability to apply precedence rules. My experience has been that after years of practice coding that's second nature.

Also, consider math. Mathematics has been around a lot longer then CS and they've adopted precedence rules and get rid of the parenthesis whenever they can.

For those reason, I prefer to eliminate as many parenthesis as possible.

\$\endgroup\$
10
\$\begingroup\$

I never put single boolean variables in parentheses, but I always put boolean expressions in parentheses (when they participate in larger expressions). It just makes the most sense to me, and I find it most readable that way. So I would write it like this:

if (bIsPaused || (runState != LnrState_Running))
{
    ....
}
\$\endgroup\$
5
\$\begingroup\$

I don't see how wrapping a single identifier into parentheses is ever going to make things clearer. On the other hand, I also don't see myself immediately understanding how a op b ? c : d parses for every possible operator op, or what a && b, c || d will evaluate to. There's also plenty of cases between that, especially when you add operators with (imho) unexpected precedence, like bitwise &.

All in all, I advise to allow your coworkers to decide which form to use where, and to spend code reviews checking code for correctness, not for compliance to formatting standards.

\$\endgroup\$
1
  • \$\begingroup\$ +1: Also code review its nice to check code coverage of unit tests to make sure it works as expected. \$\endgroup\$ – Martin York Feb 24 '12 at 19:10
2
\$\begingroup\$

I use automatic variables and abstraction to make it easy to parse for humans:

{
    boolean const bRunning = (runState == LnrState_Running);

    if (bIsPaused || !bRunning)
    {
        ....
    }
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ How about a function ProgramIsRunning() which would be something like return bIsPaused || !bRunning? Then your if statement would be if (!ProgamIsRunning()) which to me is the easiest to read. \$\endgroup\$ – Jeff Vanzella May 28 '13 at 16:19
  • 1
    \$\begingroup\$ True. It's really hard to say without knowing the other states of runState. Oh, how I wish C had nested functions. \$\endgroup\$ – Aaron S. Kurland May 28 '13 at 17:18
  • \$\begingroup\$ Or sometimes it's clearer to use unless (x) instead of if (!x), in which case you could say unless (ProgramIsRunning())... but without knowing the most common branch case, it's impossible to really suggest whether unless (x) is clearer than if (!x). \$\endgroup\$ – Todd Lehman May 29 '13 at 3:01
0
\$\begingroup\$

I think I'm agreeing with @Anton when I say use parenthesis when there is doubt. I'd say this is an issue of understandability.

If readability is the goal, then I favor more visually explicit formatting, see below. Paired characters are hard to match. The longer the line and/or the deeper the nesting the harder it becomes. For a single logic operator you can argue this example is over-formatting. There is judgement involved. But with line-leading logic operators, the logic & logic structure just pops out with just a glance.

To take this example further, I intentionally line-break as shown because it's easier to edit. I could delete the entire line starting with "||" and I don't leave a dangling operator from the line above.

if ( bIsPaused 
     || runState != LnrState_Running 
   )
{ 
    ....
}
\$\endgroup\$

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