This article was originally posted on JRoller on May 27, 2011
This is probably the worst code I ever seen.
Imagine an application with a table, where the user can filter lines according to some criterium. Someday, someone asked to be able to combine criteriums with "OR" or "AND" operands. The developer at the time thought that since the operand can only have two values, it is logical to store it as a boolean.
Some later day, the user probably complained that he can not force priority of the operands, say with parenthesis. Or maybe it was the developer's idea. So he added the possibility in the user interface to choose between eight types of operands, in a combo box: OR, AND, OR(, AND(, )OR, )AND, )OR(, )AND(. And here are the constants declaration:
public final static Boolean OR = Boolean.TRUE; public final static Boolean AND = Boolean.FALSE; public final static Boolean _OR = Boolean.FALSE; public final static Boolean _AND = Boolean.FALSE; public final static Boolean OR_ = Boolean.TRUE; public final static Boolean AND_ = Boolean.FALSE; public final static Boolean _OR_ = Boolean.TRUE; public final static Boolean _AND_ = Boolean.FALSE;
Notice how all those values still fit nicely into a Boolean. Now we need the code that parses the operand String from the combo:
static public Boolean getOperande(String aOp) { if (aOp == null) return OR; if (aOp.equals("OR")) return OR; else if (aOp.equals("AND")) return AND; else if (aOp.equals("AND(")) return AND_; else if (aOp.equals(")AND")) return _AND; else if (aOp.equals("OR(")) return OR_; else if (aOp.equals(")OR")) return _OR; else if (aOp.equals(")AND(")) return _AND_; else if (aOp.equals(")OR(")) return _OR_; else return _OR; }
And the opposite operation for displaying nicely the filter:
static public String getOperande(Boolean aOp) { if (aOp == OR) return "OR"; else if (aOp == AND) return "AND"; else if (aOp == AND_) return "AND("; else if (aOp == _AND) return ")AND"; else if (aOp == OR_) return "OR("; else if (aOp == _OR) return ")OR"; else if (aOp == _AND_) return ")AND("; else if (aOp == _OR_) return ")OR("; else return "OR"; }
Both methods apply the same principle for using at best the horizontal size of your screen. I wonder if the JVM will actually optimize out the useless if clauses...
For completeness, here is part of the code that will apply the operands:
protected boolean newApplyFilter(int i, int aRow) { boolean previousRes = true; boolean res = true; Boolean op = OR_; for (int j = i ; j < critereList.size() ; j++) { Critere c = (Critere)critereList.get(j); res = ...; if (j != i) { if (op == AND) res = previousRes && res; else if (op == OR) res = previousRes || res; else if (op == AND_) return (previousRes && newApplyFilter(j, aRow)); else if (op == OR_) return (previousRes || newApplyFilter(j, aRow)); else if (op == _AND) return (previousRes && newApplyFilter(j, aRow)); else if (op == _OR) return (previousRes || newApplyFilter(j, aRow)); else if (op == _AND_) return (previousRes && newApplyFilter(j, aRow)); else if (op == OR_) return (previousRes || newApplyFilter(j, aRow)); } previousRes = res; op = c.operande; } return res; }
That's the point where my brain just signed off, and I asked the poor guy who inherited the code to "remove the whole sh***".
I didn't know that the guy just discovered the power of regular expressions and was ready to unleash it, even for this rather simple task. Here is the parsing code he commited:
private static final Pattern patternOperator = Pattern.compile("OR|AND"); static public Boolean getOperande(String aOp) { Matcher matcherFilter = patternOperator.matcher(aOp); if (matcherFilter != null && matcherFilter.matches()) { String strOperator = matcherFilter.group(0); return ((strOperator != null) && (strOperator.equals("AND"))) ? AND : OR; } return OR; }
Yep, the eight state boolean is still the worse piece of code I ever met. Replacement code is just a minor WTF in comparison.
No comments:
Post a Comment