>Part II: Replace Nested Condition with Guard Clause

>Seems the posting I made yesterday stirred a bit of debate whether the code was more readable. First let me state, that yes I agree that .equals probably should be used, but that isn’t what was done, and I was just trying to break up the huge if then else construct when it didn’t need to be there. For reference, the sample is actually some code from the LineStyleProviderForXML in the WTP project. Again, it just happens to be the code base I’m working with the most lately. Here is the full method for your viewing pleasure:


protected TextAttribute getAttributeFor(ITextRegion region) {
/**
* a method to centralize all the "format rules" for regions
* specifically associated for how to "open" the region.
*/
// not sure why this is coming through null, but just to catch it
if (region == null) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.CDATA_TEXT);
}
String type = region.getType();
if ((type == DOMRegionContext.XML_CONTENT) || (type == DOMRegionContext.XML_DOCTYPE_INTERNAL_SUBSET)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.XML_CONTENT);
}
else if ((type == DOMRegionContext.XML_TAG_OPEN) || (type == DOMRegionContext.XML_END_TAG_OPEN) || (type == DOMRegionContext.XML_TAG_CLOSE) || (type == DOMRegionContext.XML_EMPTY_TAG_CLOSE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.TAG_BORDER);
}
else if ((type == DOMRegionContext.XML_CDATA_OPEN) || (type == DOMRegionContext.XML_CDATA_CLOSE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.CDATA_BORDER);
}
else if (type == DOMRegionContext.XML_CDATA_TEXT) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.CDATA_TEXT);
}
else if (type == DOMRegionContext.XML_TAG_ATTRIBUTE_NAME) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.TAG_ATTRIBUTE_NAME);
}
else if (type == DOMRegionContext.XML_DOCTYPE_DECLARATION) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.TAG_NAME);
}
else if (type == DOMRegionContext.XML_TAG_NAME) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.TAG_NAME);
}
else if ((type == DOMRegionContext.XML_TAG_ATTRIBUTE_VALUE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.TAG_ATTRIBUTE_VALUE);
}
else if (type == DOMRegionContext.XML_TAG_ATTRIBUTE_EQUALS) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.TAG_ATTRIBUTE_EQUALS);
}
else if ((type == DOMRegionContext.XML_COMMENT_OPEN) || (type == DOMRegionContext.XML_COMMENT_CLOSE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.COMMENT_BORDER);
}
else if (type == DOMRegionContext.XML_COMMENT_TEXT) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.COMMENT_TEXT);
}
else if (type == DOMRegionContext.XML_DOCTYPE_NAME) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.DOCTYPE_NAME);
}
else if ((type == DOMRegionContext.XML_CHAR_REFERENCE) || (type == DOMRegionContext.XML_ENTITY_REFERENCE) || (type == DOMRegionContext.XML_PE_REFERENCE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.ENTITY_REFERENCE);
}
else if (type == DOMRegionContext.XML_PI_CONTENT) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.PI_CONTENT);
}
else if ((type == DOMRegionContext.XML_PI_OPEN) || (type == DOMRegionContext.XML_PI_CLOSE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.PI_BORDER);
}
else if ((type == DOMRegionContext.XML_DECLARATION_OPEN) || (type == DOMRegionContext.XML_DECLARATION_CLOSE)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.DECL_BORDER);
}
else if (type == DOMRegionContext.XML_DOCTYPE_EXTERNAL_ID_SYSREF) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.DOCTYPE_EXTERNAL_ID_SYSREF);
}
else if (type == DOMRegionContext.XML_DOCTYPE_EXTERNAL_ID_PUBREF) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.DOCTYPE_EXTERNAL_ID_PUBREF);
}
else if ((type == DOMRegionContext.XML_DOCTYPE_EXTERNAL_ID_PUBLIC) || (type == DOMRegionContext.XML_DOCTYPE_EXTERNAL_ID_SYSTEM)) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.DOCTYPE_EXTERNAL_ID);
}
else if (type == DOMRegionContext.UNDEFINED) {
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.CDATA_TEXT);
}
else if (type == DOMRegionContext.WHITE_SPACE) {
// white space is normall not on its own ... but when it is, we'll
// treat as content
return (TextAttribute) getTextAttributes().get(IStyleConstantsXML.XML_CONTENT);
}
else {
// default, return null to signal "not handled"
// in which case, other factories should be tried
return null;
}
}

Now given the above actual code.

Would you change your original comment?
Would you refactor this using the “Replace Nested Condition with Guard Clause”?
Would you leave it as is?
If you refactored, what would your new code look like.

Now how many lines of code something has doesn’t necessarily mean that it isn’t clear or isn’t maintainable. And many times when refactoring code you do end up with a few more lines, because you break things into methods. So I would be curious to see how others might or might not handle the above code. Again, I use this as example of code that is in the wild and to foster some debate amongst the various readers on what they think is clean code. Personally, I kringe when I see that many if then else statements.

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

8 Responses to >Part II: Replace Nested Condition with Guard Clause

  1. Peter says:

    >Hello,personally, I wouldn’t touch it. Yes, I don’t like it, but if there is no need to change it (like fixing bug touching many lines), I don’t think it would be worth the effort. Intent of the code is quite clear in this case.If I would change it after all, I would probably use map, which maps ‘type’ to IStyleConstantsXML constant. I would handle other cases manually… and maybe after seeing result, I would revert it back anyway 🙂

  2. >Hi,I wouldn’t change it either. But if this is really bugging you ;-), give the ternary operator a try and remove the unnecessary parenthesis. Your code would look like this:return type == Foo || type == Bar ? o1 : type == Foo1 ? o2 : defaultValue;You can format this so that the question marks are aligned and that the last three lines are not at the same indentation as ‘return’.

  3. David Carver says:

    >I can live with the code as is, but if it needed to do more, probably would end up changing it. As for using the tertiary operator, that gets into Uncle Bob’s post “So…you want your code to be maintainable”. Just looking at the tertiary I would probably rewrite it as an If statement. 🙂

  4. AlBlue says:

    >Is your aversion to a bunch of else-if constructs because your exposure to languages has never cone across those with a special elif keyword? Or because you previously coded in a language or IDE that formatted the ‘else’ clause as one level indented which led to code randomly accellerating across the page?Having many clauses in a single guard isn’t really an issue for undestandabiliy – compare that where there are many /nested/ if statements with mixed guards (ie ifs inside ifs inside ifs) which is what I believe your reference means. Also, note that there are schools of thought that suggest only using a single return in a method (so there is only a single code flow through a block) would actively be bad. Lastly, you have here something that is returning a value. However, if you wanted to do something with that value prior to returning (validation, or putting into an array etc) then leaving the code as if-else-if is much easier to post-process a value. I think you have a more deep seated reason for not wanting to use an else-if construct, and I’d love to know what it is. Alex

  5. DaSH says:

    >I’d refactor this in a heartbeat. Extract the “type” to “IStyleConstants” mapping to either a method (using a switch statement) or a map, then you’d end up with a single call to get the value and a single “return (TextAttribute) getTextAttributes().get(style)” instead of dozens of them littering up the code. I fail to see how this refactoring isn’t an obvious choice?

  6. Lo says:

    >Like the previous commenter (dash), I fail to see how the need for refactoring isn’t obious here.I’d refactor it either the way dash offered, or suggest mapping it dynamically with a Java 5 Enum and EnumSets to have all this done in one single step.In any case, when I have to iterate over more than 3 possible cases for a similar behavior, I always end up writing a code to have this been done dynamically.1- it’s much easier to add values later, if necessary2- it’s cleaner3- it’s easier to testThough the meaning of this one is clear enough to have the refactoring not being a “priority” on the TODO list, I guess.

  7. Peter says:

    >The code isn’t nice, but it works. Does it need fixing? Probably not. Is it clear enough? Yes. Would I prefer the method to be written other way? Sure! But still, I wouldn’t change it just because I feel like changing big method to make it nicer. Proper time to refactor for me is when I need to change the method to fix the bug, or change functionality, not everytime when I see badly written method.Like lo said, refactoring this method wouldn’t be priority, just a nice-to-have.

  8. Eric Rizzo says:

    >Peter,I don’t think anyone is suggesting to go in and re-write that method just because they have nothing else to do. But at the point where you need to add yet another else if clause, that is the time to refactor so that nobody in the future has to think about what all those clauses are doing or double-check that a new one they are adding doesn’t conflict with an existing one.Personally, I would definitely do the Map refactoring – take advantage of the use of constants. If I really wanted to go far I might consider turning the constants into classes that could decide the return value on their own (but that is pretty drastic and not feasible if the constant-declaring code isn’t “touchable”).

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