>Replace Nested Condition with Guard Clauses

>In Martin Fowler’s refactoring book and the online Catalog of Refactorings, one of the items is called, “Replace Nested Conditions with Guard Clauses”.

The following code is sorta a hybrid, and is something I’ve seen in eclipse methods from time to time.


String type = region.getType();
if (type == Region.SOMETHING1) {
return somevalue;
} else if (type == Region.SOMETHING2) {
return somevalueAgain;
} else if (type == Region.SOMETHING3) {
return somevalueYetAgain;
} else if (type == Region.SOMETHING4) {
return yetAnotherSomeValue;
} else {
return null;
}

The if then else constructs are not needed, especially since if the condition is true, you leave the method anyways. These are the only statements in the entire method. Actually the method contains about 17 additional if then else conditions returning various values depending on the if check.

Why not just refactor it too:


String type = region.getType();
if (type == Region.SOMETHING1) {
return somevalue;
}

if (type == Region.SOMETHING2) {
return somevalueAgain;
}

if (type == Region.SOMETHING3) {
return somevalueYetAgain;
}

if (type == Region.SOMETHING4) {
return yetAnotherSomeValue;
}

return null;


It makes it easier to read…and the intent is clearer. Basically we have reduced the code to it’s simpliest form.

Advertisements
This entry was posted in eclipse, refactoring, xml. Bookmark the permalink.

8 Responses to >Replace Nested Condition with Guard Clauses

  1. >Hi David,just wondering: how did you get the snippets so nicely formatted?Thanks,Elias.

  2. ijuma says:

    >There’s actually a warning in the Eclipse compiler called “Unnecessary ‘else’ statement” that triggers if you have something like:if (foo) return “foo”;else return “else”;Interestingly it’s not clever enough to trigger in case you have:if (foo) return “foo”;else if (bar) return “bar”;else return “else”;By the way all the redundant braces in both versions don’t really help readability, but let’s not go into an argument about optional braces. :)Finally, I hope your code guarantees that Region.getType as well as Region.SOMETHING* always return interned Strings given the usage of ==.Ismael

  3. id says:

    >> something I've seen in eclipse > methods from time to timeNote that there is no universal set of guidelines for projects hosted at Eclipse. Each project or component defines its own guidelines.For instance, I am pretty sure that this code does not come from Eclipse Platform/Core. Not only due to the unnecessary else, but also because of the brackets around single statements.

  4. David Carver says:

    >@elias volanakis: The coloring code comes from here. They have a blogger plugin. Follow the second link on that page for more information on how to use it.@id You are correct it doesn’t come from platform.@ijuma: yes they are always interned strings. It should more accurately use .equal() to check, but again this is code that has existed for a LOOOONG time (over 3 years).

  5. AlBlue says:

    >Have you noticed that your second example is 50% bigger than your first? “clearer” is subjective, and I for one find the first much clearer. It highlights that it is an exclusive set of conditions, rather than the second in which it is less clear that it is an exclusive set, or indeed that the ordering matters. And == for strings? Not a good blog example by any means. Alex

  6. Ed Merks says:

    >I agree with Alex. Things like clarity, simplicity, and beauty are subjective. To me, an “else” makes it clear, without looking at the contained logic of the nested statements, that the two nested statements are mutually exclusive. You don’t need to look for return statements to know that you’ll never get to the second set of nested statements after having gone through the first set. To my subjective thinking, this makes the first generally more clear than the second. Do you recall those who argued that it’s clearer to have a single return statement at the end? I don’t agree with them either.

  7. Jens v.P. says:

    >Hi David,IMHO it depends on the context. I would prefer the “else”s, since “return” as well as “break” sometimes have this “goto” smell. On the other hand, I prefer readable code, and in the example the code probably is more readable with the “else”s.Jens

  8. Fabien says:

    >Well. Having multiple ‘return’ in the same method can considered as a bad practice.And using ‘==’ for String comparison is really bad. So for your example, IMHO, you should create an enum ! The you can use ‘switch’, which some people find clearer.As a matter of fact, thisif(a.equals(A)) {} else if (a.equals(B)) {} else if (a.equals(C)) {} else {}is the traditional way to write the equivalent of a ‘switch’ for Objects in Java. That’s why you often see this in eclipse sources, and that’s why an experienced programmer will understand instantly this code .

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s