February « 2007 « I’m pretty sure that’s what I mean

Multiple Return Points ARE Bad

February 8, 2007

To put this entry in context, I also write a work blog and some of my articles end up being on both my work blog and this one. So when I talk about “I got some comments about…” that probably means I got them to some entry on the other blog. Anyway…

This post is a response to an email I got making some points about #8 “Multiple Return Points” of the entry Top 10 Things I Gripe About In Code Reviews The email basically made the following points;

In the same book that I take the quote from, Fowler recommends the use of sensible multiple return points
Multiple returns might make the code harder to debug, but it’s not harder to read or understand

“Sensible” use of multiple return points

It was pointed out to me that in that same book I took a quote from, but in the section on “guardians”, Fowler recommends the use of sensible multiple returns. Whereas I agree with Fowler that multiple return points are not “the root of all evil” (I don’t know if that was the term of the email author or Fowler’s) I do think that they don’t have any place in the large mixed enterprisey development teams. (I’ll explain why later). So I admit, I agree with Fowler on some things and not on others. But that’s okay, we’re allowed to do that.

Why multiple return points are so bad

If something is hard to debug, is it hard to read?

If something is hard to debug, is it hard to understand?

I’m going to say maybe and yes to these questions. There are many reasons why something might be hard to debug, but knowing what the code was doing when the error occured should not be one of them.

If debugging is hard because you don’t know what the code did, then I stick by my claim that the code is hard to understand. Although I will conceed the “hard to read” bit, but only begrudgingly.

Let’s look at an example…

Yes, I have supplied unit tests for my example code

// Tests for the Multiple Return point business method
public void testMR_InFalseZero_ExpectFalseZero() {
  boolean enable = false;
  int counter = 0;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertFalse(mr.businessMethod());
  assertFalse(mr.isEnabled());
  assertEquals(counter, mr.getCounter());
}

public void testMR_InTrueZero_ExpectFalseOne() {
  boolean enable = true;
  int counter = 0;
  int expectedCounter = 1;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertFalse(mr.businessMethod());
  assertFalse(mr.isEnabled());
  assertEquals(expectedCounter, mr.getCounter());
}

public void testMR_InFalseFive_ExpectFalseFive() {
  boolean enable = false;
  int counter = 5;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertFalse(mr.businessMethod());
  assertFalse(mr.isEnabled());
  assertEquals(counter, mr.getCounter());
}

public void testMR_InTrueFive_ExpectTrueSix() {
  boolean enable = true;
  int counter = 5;
  int expectedCounter = 6;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertTrue(mr.businessMethod());
  assertTrue(mr.isEnabled());
  assertEquals(expectedCounter, mr.getCounter());
}

// Tests for the Single Return point business method
public void testSR_InFalseZero_ExpectFalseZero() {
  boolean enable = false;
  int counter = 0;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertFalse(mr.betterBusinessMethod());
  assertFalse(mr.isEnabled());
  assertEquals(counter, mr.getCounter());
}

public void testSR_InTrueZero_ExpectFalseOne() {
  boolean enable = true;
  int counter = 0;
  int expectedCounter = 1;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertFalse(mr.betterBusinessMethod());
  assertFalse(mr.isEnabled());
  assertEquals(expectedCounter, mr.getCounter());
}

public void testSR_InFalseFive_ExpectFalseFive() {
  boolean enable = false;
  int counter = 5;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertFalse(mr.betterBusinessMethod());
  assertFalse(mr.isEnabled());
  assertEquals(counter, mr.getCounter());
}

public void testSR_InTrueFive_ExpectTrueSix() {
  boolean enable = true;
  int counter = 5;
  int expectedCounter = 6;
  MultipleReturns mr = new MultipleReturns(enable, counter);
  assertTrue(mr.betterBusinessMethod());
  assertTrue(mr.isEnabled());
  assertEquals(expectedCounter, mr.getCounter());
}

There is nothing interesting in the above. An object is created, a method is called and the output is checked – this happens eight times. The only thing that you need to pick up on is this, the only difference between the tests called

testMR_xxx

and

testSR_xxx

is the method that they call. The two sets of tests (four each) are identical other than that.

Multiple Return Implementation

Here’s the code that is being tested;

 1:public boolean businessMethod() {
 2:  if(!isEnabled()) {
 3:    return false;
 4:  }
 5:  increment();
 6:  if(getCounter() > 5) {
 7:    return true;
 8:  } else {
 9:    flipEnabled();
10:    return false;
11:  }
12:}
13:
14:// included just to give an insight to what this method does
15:public void flipEnabled() {
16:  this.enabled = !this.enabled;
17:}

Take five minutes and try to spot the problem. It’s okay, I’ll wait.

Done? Right, here’s the answer then. Line 3 and line 10 both return the same, the difference being that line 10 is called only after it has “done stuff”. If this method exits okay, but the calling method throws an exception, it will be really hard to work out what path was taken through this particular code snippet. If false is returned, we don’t know if lines 5 and 9 were executed or not, that is going to hinder debugging.

Single Return Implementation

Compare that implementation with the following single return-point method.

 1:public boolean betterBusinessMethod() {
 2:  boolean result = false;
 3:  if(isEnabled()) {
 4:    increment();
 5:    if(getCounter() > 5) {
 6:      result = true;
 7:    } else {
 8:      flipEnabled();
 9:    }
10:  }
11:  return result;
12:}

It satisfies the exact same unit tests, but the difference is we know what path of execution happened just given the result. So if false is returned, we know that isEnabled() returned false, so none of the object state that this method affects was altered. You don’t have that luxury with the multiple return point version. You could argue that if we know the state before and the state after, we can work out what path was taken. But that just plays to my argument. I don’t claim that it’s impossible₁, I just claim that it’s more difficult. The result of true or false gives me instant understanding of the path that was taken, I don’t have to add any log statements or run the debugger, I know because it’s obvious.

So what?! This is a simple example!

This is a canned example, meaning that I wrote it simply to prove my point. But imagine now you’re scaling this example up to be part of a distributed system, or to have database or service access. Imagine that it’s interacting with many objects, rather than just itself and my point is reinforced. This complexity exists in a simple example, it will only get worse in more complex ones.

The single return method is also better when you’re on a team that involves many different developers updating the same file. Having the “only one return statement per method” policy forces all the developers to consider the change that they’re making, rather than just adding a few lines any-old-way when some work request comes in.

I suppose that it is theoretically possible to look at a spec and decide whether to use a single return or multiple return approach. But how much time will it take to perform that analysis thoroughly and completely? It would be quicker to just go with single returns everytime.

Possible bug?

Line 9 in the multiple return version may also be a mistake, maybe it was added in the wrong place? That kind of mistake is more unlikely (though not impossible) with single return methods; simply because each addition that affects the method’s result needs to be more explicitly stated rather than just slid in in some arbitary place – just adding a couple of lines with a return in means you don’t have to think (at all) about what the rest of the method may or may not be doing.

Some methods are so simple that multiple return points are okay…

There is probably a method of such simplicity that this is true, for example;

1:public boolean isGreaterThan(int number, int limit) {
2:  if(number > limit) {
3:    return true;
4:  } else {
5:    return false;
6:  }
7:}

But if this kind of operation really needs to be put in it’s own method (and that smells like a WTF to me), wouldn’t the single return version be more simple?

1:public Boolean isGreaterThan(int number, int limit) {
2:  return (number > limit);
3:}

Your example doesn’t hold because in this situation…

Before someone tells me that my unit tests aren’t complete, because in some cases where the input is this and that happens and whatever… The unit tests aren’t complete, I’m the first to admit that. So maybe the exact behaviour isn’t reflected in both implementations of the business method. But the basic subset of unit tests pass for both implementations, so isn’t this just another argument for having complete sets of unit tests before publishing your code?

It may we be that the single return point implementation doesn’t do all of what the multiple one does and that the bug is in the single return method. But that’s okay as far as I’m concerned, I’d much rather debug the single return method than the multiple one. After all, it’s easier to understand (and read…