>The Simplest Thing

>There is a great page on Ward Cunningham’s Wiki page entitled “Do the Simplest thing that Could Possibly Work“. Often times we try to over complicate items or don’t take the time to do the simplest thing. I recently had to track down an infinite loop issue that was occurring in a method on the Apache ODE project.

There was a method called extractFunction which would try to extract some unresolved functions and return that list.


/**
* Returns the list of function references in the given XPath expression
* that may not have been resolved properly, which is the case especially
* if the expression contains a preceding function, which short circuited the evaluation.
*
* @param xpathStr
* @return list of function expressions that may not have been resolved properly
*/
private List extractFunctionExprs(String xpathStr) {
ArrayList functionExprs = new ArrayList();
int firstFunction = xpathStr.indexOf("("),
lastFunction = xpathStr.lastIndexOf("(");
StringBuffer functionExpr = new StringBuffer();
if ((firstFunction > 0 && // the xpath contains a function
firstFunction < lastFunction)) { // the xpath references multiple variables
// most likely, the variable reference has not been resolved, so make that happen
boolean quoted = false, doubleQuoted = false, function = false, arguments = false;
Name11Checker nameChecker = Name11Checker.getInstance();
for (int index = firstFunction; index < xpathStr.length(); index++) {
if (!function) {
int colonIndex = xpathStr.indexOf(':', index);
if (colonIndex == -1) {
break;
}
while (colonIndex >= 0 && nameChecker.isNCNameChar(xpathStr.charAt(--colonIndex)));
if (xpathStr.charAt(colonIndex) == '$') {
index = xpathStr.indexOf(':', index) + 1;
continue;
}
function = true;
arguments = false;
functionExpr.setLength(0);
index = colonIndex;
continue;
}
char ch = xpathStr.charAt(index);
if (function) {
functionExpr.append(ch);
// in the name is qualified, don't check if its a qname when we're at the ":" character
if (ch == ':') {
continue;
} else if (ch == '(') {
if (nameChecker.isQName(functionExpr.substring(0, functionExpr.length() - 1))) {
arguments = true;
} else {
function = false;
continue;
}
} else if (ch == ')') {
if (arguments) {
function = false;
functionExprs.add(functionExpr.toString());
functionExpr.setLength(0);
}
} else {
if (!arguments) {
if (!nameChecker.isQName(functionExpr.substring(0, functionExpr.length()))) {
function = false;
}
}
}
}
}
}
return functionExprs;
}

The infinite loop happens when an XPath expression like the following is parsed:


concat("P", fnx:someTimeFunction("08:08:08.000"))

When the code hits the time stamp it will go into an infinite loop. In many cases the code is a bit more complicated and error prone than it needs to be. Is this necessarily the simplest thing that can possibly work? Also the code itself had no unit tests for the method, and it probably wasn’t designed with testing in mind to begin with since the method itself is private.

To try and address the infinite loop, I first created some unit tests to test the method under different scenarios including the infinite loop scenario. Then I rewrote the method with out changing it’s signature as follows:


private List extractFunctionExprs(String xpathStr) {
ArrayList functionExprs = new ArrayList();
// Match the prefix : function name ( all contents except the ) and the closing )'s that may occur
final String FUNCTION_REGEX = "\\w+:\\w+\\([.[^\\)]]*\\)*";
int firstFunction = xpathStr.indexOf("("),
lastFunction = xpathStr.lastIndexOf("(");
if ((firstFunction > 0 && firstFunction < lastFunction)) {
Pattern regex = Pattern.compile(FUNCTION_REGEX);
Matcher matcher = regex.matcher(xpathStr);

while (matcher.find()) {
String function = matcher.group();
functionExprs.add(function);
}
}
return functionExprs;
}

Is this full proof, probably not, but this to me is much simpler to read and understand from a maintenance stand point. The only ugly point in the code is the RegEx expression itself, but it isn’t overly complicated, but still leaves an ugly taste, but it’s something I can live with.

Writing tests will allow you to evolve your code into the simplest thing that can work. Your code will never start off that way, and if it does, they you have more programming chops than I do. Second, having tests could have caught the infinite loop error sooner rather than later. Later in this case was 6 months after the code was written and the original maintainer had moved on to other things.

The simplest thing may not necessarily be your first cut at a method, get it working, have your tests, and then take a momement to see if you can clean the code up to it’s simplest form. Your co-workers and those in the open source community that have to maintain your contributions will appreciate it.

As a side note, I rarely take in a contribution to the projects I’m a committer on with out there being some test cases to go with it. So, write the tests, your contribution may be accepted easier if you do so.

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

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