Currently refactoring WACT’s template parser, the SourceFileParser. Refactoring began with Revision 1.39.
The SourceFileParser the effectively the center of WACT’s template engine, so not screwing up is kinda important. This main issue, as you can see with Revision 1.38, is the parse() method which is a giant loop, handling all XML events (currently we’re using an XMLPull API) and, in that procedural form, has become almost impossible to modify. Hence the need to refactor…
Although I own Refactoring, can’t say I’m any kind of refactoring guru (haven’t read it from cover to cover) so attempting to apply as much “common sense” as possible, making “baby steps” rather than broad changes, and testing thoroughly with each step.
Here’s where it’s going so far;
– Have created a class TreeBuilder into which I’m dumping sections of the procedure from SourceFileParser::parse() into respective static methods.
– The names of the methods need to be as “sane” as possible, so that they “read well” as they appear in the SourceFileParser.
– Right now I don’t want to worry about dealing with an instance of TreeBuilder, and any properties it will use, hence the use of static calls. Using it as an instance from SourceFileParser is coming up in the “Round 2” of refactoring (perhaps), once I’ve finished creating static methods.
– With each new method, adding a corresponding test here, which, combined with the existing SourceFileParser tests, confirms everything is still working OK after each change. A nice side effect of this refactoring is we’re now able to examine the parsing in much more detail with unit tests.
– Not too worried right now about the design of TreeBuilder. Seems to be becoming some kind of “god class”, doing anything and everything but issues like that can wait until the main changes to the SourceFileParser are done.
One useful optimization that’s turned up already was eliminating a chunk of code which kept reappearing in SourceFileParser. Kept seeing it repeated while adding the static methods, so now methods like TreeBuilder::handlePlainTagOpen() and TreeBuilder::handlePlainTagClose() delegate some work to TreeBuilder::handlePlainText(). Otherwise the SourceFileParser::parse() method is slowly starting to be human readable.
Anyway, just some notes which might be interesting to air. Thoughts appreciated. The general evolution from large procedure to many small static methods (then later to proper objects and beyond), while ignoring issues of clever API design, seems to be quite effective for refactoring your classic hacked script.
Harry FuecksHarry Fuecks is the Engineering Project Lead at Tamedia and formerly the Head of Engineering at Squirro. He is a data-driven facilitator, leader, coach and specializes in line management, hiring software engineers, analytics, mobile, and marketing. Harry also enjoys writing and you can read his articles on SitePoint and Medium.
