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
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 –
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
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
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
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
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.