Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

How do you style complex compound AND / OR if statements for maximum readability? How do you indent and where do you place the line breaks? My particular situation is something like the following. It's definitely better than smashing everything into one line, but still looks messy.

if (
    (
        x == y
        && a != b
        && p.isGood() 
        && (
            i + u == b
            || q >= a
        )
    )
    || k.isSomething()
    || m > n
) {
    doSomething();
}
share|improve this question
1  
Is the goofy indenting and parentheses/brace structure intentional or part of the style? –  Ed S. Jun 16 '11 at 23:56
    
Funny. I asked this same question on SO a week ago and it got closed. Glad to see this question alive somewhere! –  Eric Belair Jun 17 '11 at 3:42

8 Answers 8

up vote 6 down vote accepted

Make boolean variables for each small steps:

bool step1 = i + u == b || q >= a;
bool step2 = a != b && p.isGood() && group1;
bool step3 = group2 || k.isSomething() || m > n;
if (step3) { doSomething(); }

This is of course similar to Lacrymology's answer, except with different names for each step.

If you name step1, step2 and step3 in ways that make good conceptual sense, this should be by far the most legible. p.isGood() and k.isSomething() may sometimes be invoked in situations where it wouldn't be in your original code, so this wouldn't be an option if those functions are expensive or if you're running this code in a very tight loop.

On the other hand, you needn't worry about the performance hit that creating new variables might incur; a good compiler will optimize them out.

An example with rectangle collision detection (which you probably wouldn't use due to the aforementioned performance hit):

if((a.x + a.width >= b.x || b.x + b.width >= a.x)
 && (a.y + a.height >= b.y || b.y + b.width >= a.y)
)
{ collision(); }

Might become:

bool horizMatch = a.x + a.width >= b.x || b.x + b.width >= a.x;
bool vertMatch = a.y + a.height >= b.y || b.y + b.width >= a.y;
if(horizMatch && vertMatch) { collision(); }

Also, if you want to leave your code as is, I think that would be totally fine too. I honestly think your code is quite legible. Obviously I don't know what exactly a b x y i u p k m n are, but as far as structure goes, it looks good to me.

share|improve this answer

I usually re-factor my code to be more modular if my conditionals get that complicated.

share|improve this answer
    
I would avoid refractoring in this situation. It'd be silly to test such small functions in isolation, and having the definitions dangling outside the function makes the code less self-apparent. –  Rei Miyasaka Jun 16 '11 at 23:54
    
It depends on how well you refactor. I'd argue that it makes your code far MORE self-apparent to have meaningful function names instead of a string of conditionals that looks like an algebra textbook threw up in your code. –  JohnFx Jun 17 '11 at 14:07
    
Visually though, you'd have your functions dangling outside, and it wouldn't be immediately obvious what exactly the function does. It's momentarily a black box until you scroll up. Unless you're in a language that allows functions in functions, I don't think it'd be very convenient at all either for the reader or for the writer regardless of how well you refractor. And if you're in a language that allows functions in functions, then chances are the syntax is hardly any different from declaring a binding or variable instead, e.g. let x = a > b or let f a b = a > b. –  Rei Miyasaka Jun 17 '11 at 17:18
    
Variables would work equally well. I'd consider that refactoring too. –  JohnFx Jun 17 '11 at 18:49
    
Ah, okay. –  Rei Miyasaka Jun 17 '11 at 19:07

I'd do something more like this, at this level of complexity

bool doIt = x == y && a != b && p.isGood();
doIt &= ( i + u == b || q >= a);
doIt |= k.isSomething() || m > n;

if(doIt)
{
    doSomething();
}

it's ugly, but it's readable and I'm pretty certain the compiler will know how to refactor it.

On the other hand, if I ever see myself in the situation of writing such an IF statement, I rethink the solution, because I'm CERTAIN there's a way of doing it simplier, or at least abstracting some of that condition (e.g.: maybe x == y && a != b && p.isGood() really just mean this->isPolygon() and I can make that method;

share|improve this answer

I'm getting less obsessed with vertical alignment over time, but my general form with mult-line expressions is...

if (   (   (expr1 == expr2)
        || (expr3 == expr4)
        || (expr5 == expr6)
       )
    && (   (expr7 == expr8)
        || (expr9 == expra)
       )
   )
{
  blah;
}

Key points...

  • The close parens align vertically with the open parens, as with the braces.
  • The sub-expressions that fit within one line are within one line, and are vertically aligned at the left. Where it helps readability, the infix operators within those single-line parts are vertically aligned too.
  • The closing braces naturally create nearly-blank lines, helping to visually group things.

Sometimes, I'll format + and * or some other operators like this too. Quite a few complex expressions take a sum-of-product or product-of-sum form (that can refer to boolean "sums" and "products") so it's probably common enough that a consistent style for it is worthwhile.

Be careful with this, though. It's often better to refactor (move parts of the expression into a function, or calculate and store intermediate parts in a variable) rather than using indentation to try to make an overcomplex expression more readable.

If you prefer to stack your close-parens on the right-hand-side, I don't hate it, but I guess it's not too bad. Taken too far, you run the risk that a mistake can leave the indentation misrepresenting what the parentheses do, though.

if (   (   (expr1 == expr2)
        || (expr3 == expr4)
        || (expr5 == expr6))

    && (   (expr7 == expr8)
        || (expr9 == expra)))
{
  blah;
}
share|improve this answer
    
+1 I like your styling. It answered my question directly, but I think Rei Miyasaka nailed the root of the problem. If I'm ever to lazy to do Rei's method, I'll use your styling. –  JoJo Jun 17 '11 at 4:58
    
Wow this is really nice. –  Rei Miyasaka Jun 17 '11 at 6:37

http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

I agree with JohnFx's answer as well as one by Lacrymology. I would build a bunch of functions (preferably static) that accomplish small goals and then build up on them in a smart way.

So, how about something like this? Note, this is not the perfect solution, but it works. There are ways to clean this up further, but more specific information is needed. Note: this code should run just as fast, for the compiler is smart.

// Currently based on members or global vars
// (which is often a bad idea too)
function doSomethingCondirionally()
{
  if (k.isSomething() || m > n)
  {
    doSomething();
    return;
  }

  // Else ... 
  if (x != y) return;
  if (a == b) return;
  if (!p.isGood()) return;

  // Final, positive check
  if (i + u == b || q >= a)
  {
    doSomething();
  }
}
share|improve this answer
    
If having only one exit point from a function is something you value (e.g. if you're in a functional language), this might not be the best option, or even an available one. That said, yeah, this is what I often do. –  Rei Miyasaka Jun 16 '11 at 23:30
    
What if it's an interpreted language like Javascript? –  JoJo Jun 16 '11 at 23:34
    
@ Rei Miyasaka, how much I value it depends on a language. While I like the LISP family of languages, I have not had to use one at work. If I have to re-factor someone else's function but touch no other code (often a reality), then I would do something like above. If I can write/rewrite this logic from ground up, then my approach will be different, but I cannot write such code without having a specific example of what the author is trying to do here. –  Job Jun 16 '11 at 23:48
1  
@Rei Miyasaka, That person might be a genius or be full of crap. I do not know everything, but I would be curious to know that person's defense of single-exit point. There is some discussion of that here and on SO, and the impression I got was that this approach has been popular among academics in the 80s maybe, but no longer matters, and can in fact hinder readability. Of course, if you are doing everything in LINQ functional-style, then this problem would not even come up. –  Job Jun 16 '11 at 23:59
2  
@Job @Steve I think it's a more important guideline in languages that require explicit deallocation. Obviously not for every function, but it's probably a habit that beginner programmers were encouraged to hold in order to avoid forgetting to free resources. –  Rei Miyasaka Jun 17 '11 at 6:25

For what it's worth, I was surprised to see that your example looks a lot like the complicated predicates I've written. I agree with others that a complicated predicate isn't the greatest for maintainability or readability, but occasionally they come up.

Let me emphasize that you did this part correct: && a != b NEVER put logical connector at the end of a line, it's too easy to miss visually. Another place where you should NEVER put an operator at the end of the line is in string concatenation, in languages with such an operator.

Do this:

String a = b
   + "something"
   + c
   ;

Don't do this:

String a = b +
   "something" +
   c;
share|improve this answer
    
Do you have any logic or studies supporting your assertion for the logical connectors, or is it merely your preference stated as fact? I've heard this a lot in various places, and never understood it (unlike yoda conditionals, which have a valid [if misguided] reason). –  Caleb Huitt - cjhuitt Jun 17 '11 at 1:17
    
@Caleb - Mathematics has been typeset in that fashion for centuries. When skimming code, we focus on the left-hand side of each line. A lines beginning with an operator is obviously a continuation of the previous line, and not an incorrectly indented new statement. –  kevin cline Jun 17 '11 at 3:47
    
I also like prefixing of mathematical operators. I realized it way to late in my programming career :) –  JoJo Jun 17 '11 at 5:01

If the conditional is that complicated, it is usually an indication that it should be broken up into parts. Perhaps one clause can be assigned to an intermediate variable. Perhaps one clause can be turned into a helper method. I generally prefer not to have so many ands and ors in one line.

share|improve this answer

You could break up the code into multiple statements, making it easy to understand. But a real ninja would do something like this. :-)

if
(
    (
        x == y
    &&
        a != b
    &&
        p.isGood()
    &&
        (
            i + u == b
        ||
            q >= a
        )
    )
||
    k.isSomething()
||
    m > n
)
{
    doSomething();
}
share|improve this answer
5  
I'm a fan of whitespace, but this is excessively padded out with nearly-blank lines for my taste. –  Steve314 Jun 17 '11 at 1:36

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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