Short, concise and readable code - invert your logic and stop nesting already!

Have you ever heard this maxim:

"A method should have one and only one exit point"

Well, it couldn't be more wrong. The following are the attributes of a well written method; it should:

• perform one and only one function,
• be short and to the point,
• be easy to read and understand,
• and it should have a descriptive, accurate and concise name.

Notice that none of these points says anything about how or where a method should exit. To write a clean, easy to read method, follow these guidelines:

• follow a template. consistent flow is easier to read.
• check your pre conditions early. If they fail, exit fast
• nesting is bad. avoid it. invert your logic at every step of the way.

First, the template:

[return value] [method name](parameters)
[throws clause]
{
  [check pre conditions]

  [core logic]
}

Pretty simple, right? So what's this about invert your logic? Well have a look at the template up there. Do you see any nesting? Right....neither do I. So let me illustrate a common idiom, one that uses in particular the if/else if/else pattern and the single exit strategy:

/**
* Returns the element at index
*/
public Object getElement(int index)
{
  Object theItem = null;
  boolean listError = false;
  boolean indexError = false;

  if (list != null) {
    if (index > 0 && index < list.size()) {
      theItem = list.elementAt(index);
    } else {
      indexError = true;
    }
  } else {
    listError = true;
  }

  if (listError) {
    throw new Exception("Bad list");
  } else if (indexError) {
    throw new IndexOutOfBoundsException("Bad index");
  } else {
    return theItem;
  }
} 

Wow, what a mouthful. And I didn't even put in a few while loops, case structures and other nonsense I usually see that ends up with code nested 4 and 6 levels deep. Let me rewrite that code up there making it match the pattern I suggested, inverting the logic, and then I will explain what I did.

/**
* Returns the element at index
*/
public Object getElement(int index)
{
   if (list == null) {
       throw new Exception("Bad list");
   }

   if (index < 0 || index >= list.size()) {
       throw new IndexOutOfBoundsException("Bad index");
   }

   return list.elementAt(index);
}

Remember when I said check your pre-conditions first, and exit fast. What this allows you to do is evaluate all of the conditions under which your method will fail, and if you detect something amiss, handle it immediately. This strategy is flexible - if the pre-conditions for your class or your method change, you can add and substract the tests for those pre-conditions using this structure without having to modify surrounding code. The worst offender I see is always of the following pattern:

if (condition_to_succeed_is_true) {
   do_something();
} else {
   do_error();
}

The problem with this is that the reader of your code has to put the conditional test onto their mental stack while they digest what do_something() is doing. If do_something() happens to be long, or complicated, you'll probably blow the mental stack of the reader, forcing them to look at the condition again just to figure out why the do_error() is being done.

On the other hand, when you line up your pre-condition tests linearly, there is no mental stack, the reader simply processes each in turn, and then, when they are all done, they are able to process the real meat - the do_something() - part of your method without all the baggage from your pre-condition tests. So inverting your logic means taking the above test, inverting the condition, and writing it as:

if (!condition_to_succeed_is_true) {
   do_error();
   return;
}

do_something();

So I hope you remember your CS classes and De Morgan's laws - I find coding like this makes me use them all the time.

There's one other benefit this strategy has, and that's when you are writing synchronized blocks. When you write a synchronized block, you absolutely must strive to perform as little work as is absolutely necessary inside the synchronized block.

Let's look at the problem we have when we use the normal pattern I described above, combined with synchronization -- the code now becomes:

synchronized (lock) {
   if (condition_to_succeed_is_true) {
       do_something();
   } else {
       do_error();
   }
}

Ugggh! There's no way, in Java, to release the lock before you perform do_something()! (Which we presume takes time and shouldn't be performed under lock). If you invert the logic, however, you can test the condition and release the lock as soon as you've tested it (note that it's often the case that you might need to use some data you acquired during the lock, in that case you should make a copy on the local stack under the lock, and then release it which I have shown below):

synchronized (lock) {
   if (!condition_to_succeed_is_true) {
       do_error();
       return;
   }
   state = copy_state();
}

do_something(state);

Remember, in these examples I am assuming that do_something(...) is the non trivial part of your method, both in lines of code, complexity, and execution time.

One more thing - I find that using 4 spaces to indent code blocks instead of 2 helps to break me of the habit of nesting my code because it acts like an automatic brake on the indentation level.