14

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();
}
2
  • 1
    Is the goofy indenting and parentheses/brace structure intentional or part of the style? Jun 16, 2011 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! Jun 17, 2011 at 3:42

8 Answers 8

7

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.

9

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

5
  • 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. Jun 16, 2011 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, 2011 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. Jun 17, 2011 at 17:18
  • Variables would work equally well. I'd consider that refactoring too.
    – JohnFx
    Jun 17, 2011 at 18:49
  • Ah, okay. Jun 17, 2011 at 19:07
8

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;

4

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;
}
2
  • +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, 2011 at 4:58
  • Wow this is really nice. Jun 17, 2011 at 6:37
1

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;
3
  • 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). Jun 17, 2011 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. Jun 17, 2011 at 3:47
  • I also like prefixing of mathematical operators. I realized it way to late in my programming career :)
    – JoJo
    Jun 17, 2011 at 5:01
1

https://blog.codinghorror.com/flattening-arrow-code/

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();
  }
}
10
  • 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. Jun 16, 2011 at 23:30
  • What if it's an interpreted language like Javascript?
    – JoJo
    Jun 16, 2011 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, 2011 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, 2011 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. Jun 17, 2011 at 6:25
0

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.

0

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();
}
1
  • 5
    I'm a fan of whitespace, but this is excessively padded out with nearly-blank lines for my taste.
    – user8709
    Jun 17, 2011 at 1:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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