>Part III: Replace Nested Conditionals with Guard Clause

>AlBlue asked a simple question, what my aversion to if then else constructs is. It isn’t so much an aversion but the abuse of it. First let me say I’m a jack of all trades, I’ve programmed in C, C++, COBOL, IBM 370 Assembler, C#, Perl, Visual Basic, Java, and numerous other languages and seen the simple if then else constructs go from being fairly simple in the original implementation to full blown chaos.

I also used to be of the mind set that one always had One Entry point and One Exit point, however I’m not convinced that it is necessarily the best situation to use in modern programming languages. Anyway, AlBlue to answer your question…I’ve seen the example explode into the following as more functionality is added (again, this is real code and I’m fairly positive it started off much simplier than the final implementation):


private DOMRegion formatStartTag(TextEdit textEdit, Position formatRange, XMLFormattingConstraints parentConstraints, DOMRegion currentDOMRegion, IStructuredDocumentRegion previousDocumentRegion) {
// determine proper indent by referring to parent constraints,
// previous node, and current node
IStructuredDocumentRegion currentDocumentRegion = currentDOMRegion.documentRegion;
IDOMNode currentDOMNode = currentDOMRegion.domNode;

// create a constraint for this tag
XMLFormattingConstraints thisConstraints = new XMLFormattingConstraints();
XMLFormattingConstraints childrenConstraints = new XMLFormattingConstraints();
updateFormattingConstraints(parentConstraints, thisConstraints, childrenConstraints, currentDOMRegion);

if(childrenConstraints.getWhitespaceStrategy() == XMLFormattingConstraints.DEFAULT)
childrenConstraints.setWhitespaceStrategy((new XMLFormattingPreferences()).getElementWhitespaceStrategy());

String whitespaceStrategy = thisConstraints.getWhitespaceStrategy();
String indentStrategy = thisConstraints.getIndentStrategy();
int availableLineWidth = thisConstraints.getAvailableLineWidth();

// format space before start tag
// do not format space before start tag if preserving spaces
if (whitespaceStrategy != XMLFormattingConstraints.PRESERVE) {
// format like indent strategy says
if (indentStrategy == XMLFormattingConstraints.INDENT || indentStrategy == XMLFormattingConstraints.NEW_LINE) {
availableLineWidth = indentIfPossible(textEdit, thisConstraints, currentDocumentRegion, previousDocumentRegion, whitespaceStrategy, indentStrategy, true);
if (availableLineWidth > 0)
thisConstraints.setAvailableLineWidth(availableLineWidth);
}
}
// format the start tag itself
boolean tagEnded = formatWithinTag(textEdit, thisConstraints, currentDocumentRegion, previousDocumentRegion);

// format children
if (!tagEnded) {
// update childConstraints with thisConstraint's indentLevel &
// availableLineWidth
childrenConstraints.setIndentLevel(thisConstraints.getIndentLevel());
childrenConstraints.setAvailableLineWidth(thisConstraints.getAvailableLineWidth());

previousDocumentRegion = currentDocumentRegion;
IDOMNode childDOMNode = (IDOMNode) currentDOMNode.getFirstChild();
IStructuredDocumentRegion nextRegion = currentDocumentRegion.getNext();
boolean passedFormatRange = false;
// as long as there is one child
if (childDOMNode != null && nextRegion != null) {
while (childDOMNode != null && nextRegion != null && !passedFormatRange && (fProgressMonitor == null || !fProgressMonitor.isCanceled())) {
DOMRegion childDOMRegion = new DOMRegion();
childDOMRegion.documentRegion = nextRegion;
childDOMRegion.domNode = childDOMNode;
if (nextRegion == childDOMNode.getFirstStructuredDocumentRegion()) {
// format children. pass in child constraints
childDOMRegion = formatRegion(textEdit, formatRange, childrenConstraints, childDOMRegion, previousDocumentRegion);
}
else {
// TODO: what happens if they dont match up?
}

// update childDOMRegion with next dom/region node
if (childDOMRegion.domNode != null) {
childDOMNode = (IDOMNode) childDOMRegion.domNode.getNextSibling();
}
else {
childDOMNode = null;
}
previousDocumentRegion = childDOMRegion.documentRegion;
nextRegion = previousDocumentRegion.getNext();
if (nextRegion != null)
passedFormatRange = !formatRange.overlapsWith(nextRegion.getStartOffset(), nextRegion.getLength());
}
}
else {
// there were no children, so keep end tag inlined
childrenConstraints.setWhitespaceStrategy(XMLFormattingConstraints.COLLAPSE);
childrenConstraints.setIndentStrategy(XMLFormattingConstraints.INLINE);
}

if (!passedFormatRange) {
// update the dom region with the last formatted region/dom
// node should be end tag and this tag's DOMNode
currentDOMRegion.documentRegion = nextRegion;
currentDOMRegion.domNode = currentDOMNode;

// end tag's indent level should be same as start tag's
childrenConstraints.setIndentLevel(thisConstraints.getIndentLevel());
// format end tag
boolean formatEndTag = false;
if (nextRegion != null && currentDOMNode != null) {
ITextRegionList rs = nextRegion.getRegions();
if (rs.size() > 1) {
ITextRegion r = rs.get(0);
if (r != null && r.getType() == DOMRegionContext.XML_END_TAG_OPEN) {
r = rs.get(1);
if (r != null && r.getType() == DOMRegionContext.XML_TAG_NAME) {
String tagName = nextRegion.getText(r);
if (tagName != null && tagName.equals(currentDOMNode.getNodeName()))
formatEndTag = true;
}
}

}
}
if (formatEndTag)
formatEndTag(textEdit, formatRange, childrenConstraints, currentDOMRegion, previousDocumentRegion);
else {
// missing end tag so return last formatted document
// region
currentDOMRegion.documentRegion = previousDocumentRegion;
}
}
else {
// passed format range before could finish, so update dom
// region to last known formatted region
currentDOMRegion.documentRegion = nextRegion;
currentDOMRegion.domNode = childDOMNode;
}

// update parent constraint since this is what is passed back
parentConstraints.setAvailableLineWidth(childrenConstraints.getAvailableLineWidth());
}
else {
// update available line width
parentConstraints.setAvailableLineWidth(thisConstraints.getAvailableLineWidth());
}
return currentDOMRegion;
}

The issue is not necessarily my aversion to If-then-Else constructs, it’s how they can grow and fester, making it much harder for somebody to add or change the code without introducing unknown bugs. I agree refactoring the original example that started this discussion would probably require the function to be changed if it needed additional functionality, but it would also hopefully allow that change to keep the clarity of the code and intent, while still allowing for the additional functionality. Making sure that code is maintainable is just as important as getting it working, especially in an open source environment where we are trying to get people to help contribute and improve the project. Ugly code is just a barrier to entry that can be eliminated.

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

5 Responses to >Part III: Replace Nested Conditionals with Guard Clause

  1. >Yes, these large conditional constructs just cry out for Scala’s pattern-matching with case classes. They provide just enough encapsulation of the variation points of behaviour, while still providing the flexibility of applying conditional logic externally.The best thing about case-classes is that they preserve the single-exit rule, to which I am an adherent. 😉

  2. AlBlue says:

    >Right, this example is ugly. But it’s also completely different from the first example in which you proposed changing a perfectly normal multi-case switch-like if block into a multi-returned disjointed method for no particular reason. Yes, ifs-within-ifs can be bad, but all code can go bad. Refactoring non-bad code is a bit like unneccesary plastic surgery; it’s being done for the sake of it, potentially introducing risk for no practical benefit. Other languages (ML, Prolog, Haskell and of course Scala as your other commenter mentioned) have the ability to have a one-of-many choice operations as core language operations. Sadly, Java can only emulate this with an if-else-if construct; but that pattern in itself is so common it is immediately understandable. I don’t see a if-else-if construct necessarily evolving into a spaghetti mess of if-within-if example given above. Quite likely, if you code a one-of-choice list you can easily introduce guards such that you maintain that 1-deep property of the true block. It’s not if-else-if that’s the problem, it’s if-if-if. As long as the true block doesn’t contain an if, and the false block is an else-if, then it’s perfectly fine. Of course, opinions vary but I think your first proposed refactoring takes it too far in the abolision of all else clauses for the sake of it.Alex

  3. David Carver says:

    >Alex you are correct, sometimes it is better to not touch things. I can live with the code as is…but think there are better ways in which the same code can be written. The main thing here is that people tend to let something simple become a mess.If I’ve managed to at least get people working on eclipse code to pause…think about it…and then either agree or reject it. At least they are thinking about it.How much refactoring one does is a matter of personal taste, but I think we can all agree that certain parts of eclipse could use some code refactoring to reduce some of the bloat and make it easier to maintain.

  4. AlBlue says:

    >Yeah, Eclipse could use a spring cleaning. And let’s not forget Yegge’s “kingdom of nouns” essay, which could easily be just about Eclipse …Alex

  5. ijuma says:

    >Regarding the first comment that mentions pattern matching with case classes in Scala, that’s a bit misleading. If you can create case classes, then you can also create enums and use a switch statement in Java (although the Scala version is a bit nicer because there is no fall-through).The advantage of Scala in this respect is how flexible pattern matching is, independently of case classes (they are not needed for pattern matching). Things like extractors as well as constructor, sequence, tuple and typed patterns.

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