This 30-day project explores the refactoring of a legacy system. The Everything Engine is an aging software project that powers Perl Monks, Everything 2, and a few other websites. It suffers from poor design and maintainiability. Learn what it’s like to look over the shoulder of an experienced developer as he refactors, redesigns, and updates the code.

Fixing the Setting Test

The big goal for today is to fix the setting test. Earlier I had the idea to make sure that all of the node classes have methods that return the associated table (a list of them anyway) and all of the public and private attribute names. That seems like a good next step to get this configuration information (and all of the SQL) out of the nodes and where it belongs.

First, why is the setting test failing? The first failed test checks to see if Everything::Node::setting loads several other modules. If that’s even useful, there are better ways to handle it — by checking %INC after loading the module, for example.

It appears that most of the remaining tests fail because the test itself is awkward. Using Test::MockObject::Extends instead of FakeNode should make that better. After fixing three failing tests, it appears like the right approach. (Of course, that leaves 80% of the file to edit, but it’s by this point almost a mechanical translation; I remember why this code was ugly and how much it made me want to write Test::MockObject in the first place.)

At 25% of the way through converting the test file, the hated *ToXML() functions start appearing. This should really be somewhere else. Nodes should represent their data, not particular views of the data. Someday I’ll move this somewhere else – perhaps under Everything::NodeBase::XML. For now, because I don’t want the code to break, I’ll make the tests pass and dream….

Oh joy, the code uses indirect object syntax with the constructor. Not only is it using the hateful XML::DOM, mixing presentation with the model, but it’s using nasty evil syntax I hate. (Do I sound especially grumpy here? I just filed a bug against a stupid regression in another system I used after someone broke lots of things I had working already.)

Hm, in making this work I found a bug in Test::MockObject::Extends, specifically that its mock() method does not return the object. That means that chained mocks don’t work. Weird. Why didn’t I notice it before? Oh well – it goes on the list of bugs to fix in that module.

Wow, the test for applyXMLFix() is huge. I wonder if I can get it shorter. I’m not really trying for that, though. I’m just trying to make the test pass (and slightly more maintainable) with as little work as possible.

As expected, most of the remaining tests were easy to translate just by migrating the code to Test::MockObject. If you look at the diff, note that this meant that I could delete the AUTOLOAD() subroutine that exploited Perl’s fuction and method calling. That’s the big secret that made the tests pass.

I’m a lot happier with the test now, and a bit happier with the code as I took the opportunity to clean up some of the control flow. It’s a small thing, but returning early from a method when there’s nothing to do reads much more nicely to me than looking through nested conditionals (especially with very short else clauses).

According to the clock, the hour is up. This seemed like a short day. I didn’t get the chance to fill in the metadata, but all tests of the system now pass. That’s definitely progress. The diff for this version (checkin #834) as well out to show how to translate the existing tests to Test::MockObject-style tests, which is a nice small task for an interested hacker.

On the whole, I think I wrote negative lines of code today. That’s always nice. It’s actually a goal of the project — to refactor out a lot of repeated and near-repeated code. Cleaning up some of the small parts makes it easier to do so.