title: How to split up complex conditions and keep short circuit evaluation?
body: <p>Sometimes conditions can become quite complex, so for readability I usually split them up and give each component a meaningful name. This defeats short-circuit evaluation however, which can pose a problem. I came up with a wrapper approach, but it is way too verbose in my opinion. </p>
  
  <p>Can anybody come up with a neat solution for this?</p>
  
  <p>See code below for examples of what I mean:</p>
  
  <pre><code>public class BooleanEvaluator {
  
      // problem: complex boolean expression, hard to read
      public static void main1(String[] args) {
  
          if (args != null &amp;&amp; args.length == 2 &amp;&amp; !args[0].equals(args[1])) {
              System.out.println("Args are ok");
          }
      }
  
      // solution: simplified by splitting up and using meaningful names
      // problem: no short circuit evaluation
      public static void main2(String[] args) {
  
          boolean argsNotNull = args != null;
          boolean argsLengthOk = args.length == 2;
          boolean argsAreNotEqual = !args[0].equals(args[1]);
  
          if (argsNotNull &amp;&amp; argsLengthOk &amp;&amp; argsAreNotEqual) {
              System.out.println("Args are ok");
          }
      }
  
      // solution: wrappers to delay the evaluation 
      // problem: verbose
      public static void main3(final String[] args) {
  
          abstract class BooleanExpression {
              abstract boolean eval();
          }
  
          BooleanExpression argsNotNull = new BooleanExpression() {
              boolean eval() {
                  return args != null;
              }
          };
  
          BooleanExpression argsLengthIsOk = new BooleanExpression() {
              boolean eval() {
                  return args.length == 2;
              }
          };
  
          BooleanExpression argsAreNotEqual = new BooleanExpression() {
              boolean eval() {
                  return !args[0].equals(args[1]);
              }
          };
  
          if (argsNotNull.eval() &amp;&amp; argsLengthIsOk.eval() &amp;&amp; argsAreNotEqual.eval()) {
              System.out.println("Args are ok");
          }
      }
  }
  </code></pre>
  
  <p><strong>Response to answers:</strong></p>
  
  <p>Thanks for all your ideas! The following alternatives were submitted so far:</p>
  
  <ul>
  <li>Break lines and add comments</li>
  <li>Leave as is</li>
  <li>Extract methods</li>
  <li>Early returns</li>
  <li>Nested / Split up if's</li>
  </ul>
  
  <p><strong>Break lines and add comments:</strong></p>
  
  <p>Just adding linebreaks within a condition gets undone by the code formatter in Eclipse (ctrl+shift+f). Inline comments helps with this, but leaves little space on each line and can result in ugly wrapping. In simple cases this might be enough however.</p>
  
  <p><strong>Leave as is:</strong></p>
  
  <p>The example condition I gave is quite simplistic, so you might not need to address readability issues in this case. I was thinking of situations where the condition is much more complex, for example:</p>
  
  <pre><code>private boolean targetFound(String target, List&lt;String&gt; items,
          int position, int min, int max) {
  
      return ((position &gt;= min &amp;&amp; position &lt; max &amp;&amp; ((position % 2 == 0 &amp;&amp; items
              .get(position).equals(target)) || (position % 2 == 1)
              &amp;&amp; position &gt; min &amp;&amp; items.get(position - 1).equals(target)))
              || (position &lt; min &amp;&amp; items.get(0).equals(target)) || (position &gt;= max &amp;&amp; items
              .get(items.size() - 1).equals(target)));
  }
  </code></pre>
  
  <p>I would not recommend leaving this as it is.    </p>
  
  <p><strong>Extract methods:</strong></p>
  
  <p>I considered extracting methods, as was suggested in several answers. The disadvantage of that is that these methods typically have a very low granularity and may not be very meaningful by themselves, so it can clutter your class, for example:</p>
  
  <pre><code>private static boolean lengthOK(String[] args) {
      return args.length == 2;
  }
  </code></pre>
  
  <p>This would not really deserve to be a separate method at class level. Also you have to pass all the relevant arguments to each method. If you create a separate class purely for evaluating a very complex condition then this might be an ok solution IMO.</p>
  
  <p>What I tried to achieve with the BooleanExpression approach is that the logic remains local. Notice that even the declaration of BooleanExpression is local (I don't think I've ever come across a use-case for a local class declaration before!).</p>
  
  <p><strong>Early returns:</strong></p>
  
  <p>The early returns solution seems adequate, even though I don't favor the idiom. An alternative notation:</p>
  
  <pre><code>public static boolean areArgsOk(String[] args) {
  
      check_args: {
          if (args == null) {
              break check_args;
          }
          if (args.length != 2) {
              break check_args;
          }
          if (args[0].equals(args[1])) {
              break check_args;
          }
          return true;
      }
      return false;
  }
  </code></pre>
  
  <p>I realize most people hate labels and breaks, and this style might be too uncommon to be considered readable.</p>
  
  <p><strong>Nested/split up if's:</strong></p>
  
  <p>It allows the introduction of meaningful names in combination with optimized evaluation. A drawback is the complex tree of conditional statements that can ensue</p>
  
  <p><strong>Showdown</strong></p>
  
  <p>So to see which approach I utlimately favor, I applied several of the suggested solutions to the complex targetFound example presented above. Here are my results:</p>
  
  <p><strong>nested / split if's, with meaningful names</strong>
  very verbose, meaningful names don't really help the readability here </p>
  
  <pre><code>private boolean targetFound1(String target, List&lt;String&gt; items,
          int position, int min, int max) {
  
      boolean result;
      boolean inWindow = position &gt;= min &amp;&amp; position &lt; max;
      if (inWindow) {
  
          boolean foundInEvenPosition = position % 2 == 0
                  &amp;&amp; items.get(position).equals(target);
          if (foundInEvenPosition) {
              result = true;
          } else {
              boolean foundInOddPosition = (position % 2 == 1)
                      &amp;&amp; position &gt; min
                      &amp;&amp; items.get(position - 1).equals(target);
              result = foundInOddPosition;
          }
      } else {
          boolean beforeWindow = position &lt; min;
          if (beforeWindow) {
  
              boolean matchesFirstItem = items.get(0).equals(target);
              result = matchesFirstItem;
          } else {
  
              boolean afterWindow = position &gt;= max;
              if (afterWindow) {
  
                  boolean matchesLastItem = items.get(items.size() - 1)
                          .equals(target);
                  result = matchesLastItem;
              } else {
                  result = false;
              }
          }
      }
      return result;
  }
  </code></pre>
  
  <p><strong>nested / split if's, with comments</strong>
  less verbose, but still hard to read and easy to create bugs</p>
  
  <pre><code>private boolean targetFound2(String target, List&lt;String&gt; items,
          int position, int min, int max) {
  
      boolean result;
      if ((position &gt;= min &amp;&amp; position &lt; max)) { // in window
  
          if ((position % 2 == 0 &amp;&amp; items.get(position).equals(target))) {
              // even position
              result = true;
          } else { // odd position
              result = ((position % 2 == 1) &amp;&amp; position &gt; min &amp;&amp; items.get(
                      position - 1).equals(target));
          }
      } else if ((position &lt; min)) { // before window
          result = items.get(0).equals(target);
      } else if ((position &gt;= max)) { // after window
          result = items.get(items.size() - 1).equals(target);
      } else {
          result = false;
      }
      return result;
  }
  </code></pre>
  
  <p><strong>early returns</strong>
  even more compact, but the conditional tree remains just as complex</p>
  
  <pre><code>private boolean targetFound3(String target, List&lt;String&gt; items,
          int position, int min, int max) {
  
      if ((position &gt;= min &amp;&amp; position &lt; max)) { // in window
  
          if ((position % 2 == 0 &amp;&amp; items.get(position).equals(target))) {
              return true; // even position
          } else {
              return (position % 2 == 1) &amp;&amp; position &gt; min &amp;&amp; items.get(
                      position - 1).equals(target); // odd position
          }
      } else if ((position &lt; min)) { // before window
          return items.get(0).equals(target);
      } else if ((position &gt;= max)) { // after window
          return items.get(items.size() - 1).equals(target);
      } else {
          return false;
      }
  }
  </code></pre>
  
  <p><strong>extracted methods</strong>
  results in nonsensical methods in your class
  the parameter passing is annoying</p>
  
  <pre><code>private boolean targetFound4(String target, List&lt;String&gt; items,
          int position, int min, int max) {
  
      return (foundInWindow(target, items, position, min, max)
              || foundBefore(target, items, position, min) || foundAfter(
              target, items, position, max));
  }
  
  private boolean foundAfter(String target, List&lt;String&gt; items, int position,
          int max) {
      return (position &gt;= max &amp;&amp; items.get(items.size() - 1).equals(target));
  }
  
  private boolean foundBefore(String target, List&lt;String&gt; items,
          int position, int min) {
      return (position &lt; min &amp;&amp; items.get(0).equals(target));
  }
  
  private boolean foundInWindow(String target, List&lt;String&gt; items,
          int position, int min, int max) {
      return (position &gt;= min &amp;&amp; position &lt; max &amp;&amp; ((position % 2 == 0 &amp;&amp; items
              .get(position).equals(target)) || (position % 2 == 1)
              &amp;&amp; position &gt; min &amp;&amp; items.get(position - 1).equals(target)));
  }
  </code></pre>
  
  <p><strong>BooleanExpression wrappers revisited</strong>
  note that the method parameters must be declared final
  for this complex case the verbosity is defendable IMO
  Maybe closures will make this easier, if they ever agree on that (-</p>
  
  <pre><code>private boolean targetFound5(final String target, final List&lt;String&gt; items,
          final int position, final int min, final int max) {
  
      abstract class BooleanExpression {
          abstract boolean eval();
      }
  
      BooleanExpression foundInWindow = new BooleanExpression() {
  
          boolean eval() {
              return position &gt;= min &amp;&amp; position &lt; max
                      &amp;&amp; (foundAtEvenPosition() || foundAtOddPosition());
          }
  
          private boolean foundAtEvenPosition() {
              return position % 2 == 0 &amp;&amp; items.get(position).equals(target);
          }
  
          private boolean foundAtOddPosition() {
              return position % 2 == 1 &amp;&amp; position &gt; min
                      &amp;&amp; items.get(position - 1).equals(target);
          }
      };
  
      BooleanExpression foundBefore = new BooleanExpression() {
          boolean eval() {
              return position &lt; min &amp;&amp; items.get(0).equals(target);
          }
      };
  
      BooleanExpression foundAfter = new BooleanExpression() {
          boolean eval() {
              return position &gt;= max
                      &amp;&amp; items.get(items.size() - 1).equals(target);
          }
      };
  
      return foundInWindow.eval() || foundBefore.eval() || foundAfter.eval();
  }
  </code></pre>
  
  <p>I guess it really depends on the situation (as always). For very complex conditions the wrapper approach might be defendable, although it is uncommon. </p>
  
  <p>Thanks for all your input!</p>
  
  <p>EDIT: afterthought. It might be even better to create a specific class for complex logic such as this:</p>
  
  <pre><code>import java.util.ArrayList;
  import java.util.List;
  
  public class IsTargetFoundExpression {
  
      private final String target;
      private final List&lt;String&gt; items;
      private final int position;
      private final int min;
      private final int max;
  
      public IsTargetFoundExpression(String target, List&lt;String&gt; items, int position, int min, int max) {
          this.target = target;
          this.items = new ArrayList(items);
          this.position = position;
          this.min = min;
          this.max = max;
      }
  
      public boolean evaluate() {
          return foundInWindow() || foundBefore() || foundAfter();
      }
  
      private boolean foundInWindow() {
          return position &gt;= min &amp;&amp; position &lt; max &amp;&amp; (foundAtEvenPosition() || foundAtOddPosition());
      }
  
      private boolean foundAtEvenPosition() {
          return position % 2 == 0 &amp;&amp; items.get(position).equals(target);
      }
  
      private boolean foundAtOddPosition() {
          return position % 2 == 1 &amp;&amp; position &gt; min &amp;&amp; items.get(position - 1).equals(target);
      }
  
      private boolean foundBefore() {
          return position &lt; min &amp;&amp; items.get(0).equals(target);
      }
  
      private boolean foundAfter() {
          return position &gt;= max &amp;&amp; items.get(items.size() - 1).equals(target);
      }
  }
  </code></pre>
  
  <p>The logic is complex enough to warrant a separate class (and unit test, yay!). It will keep the code using this logic more readable and promotes reuse in case this logic is needed elsewhere. I think this is a nice class because it truely has a single responsibility and only final fields.</p>