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.
Today’s task is to unentangle the database code from the nodebase code. This is the biggest and most valuable refactoring yet.
Day 24: The Great Database Reshuffling
Today, I can finally move database code elsewhere. The first thing to do
is to create
Everything::Storage and its subclasses.
$ svk mkdir lib/Everything/Storage
Everything::Storage now can be only:
package Everything::Storage; use strict; use warnings; 1;
I was about to explain that this allows subclassing for multiple storage
types, but I don’t need that right now, so I’ll just use
Everything::Storage as the SQL storage engine for now and add
subclasses when I need them, if ever.
$ svk revert lib/Everything/Storage
That’s better. The name of the module should also be
Everything::DB instead. (It can become
Everything::Storage::DB if that day ever arrives.)
What should this new module contain? All of the database-touching code from
How will it maintain the
Everything::NodeBase interface? By
exporting all of the moved methods into the namespace for now. It’s not a
perfect solution, but it should allow all of the tests to pass.
(Ultimately, it’s probably best to make
role provided by a more specific role implementation, then create a
role-decorated class for each usable storage engine. First, this
Everything::NodeBase into the new file, then
deleted everything except the methods I want to keep. Then I added
Everything::DB and used it from
Everything::NodeBase. Finally, I removed all of the methods in
Everything::NodeBase that are in the new file.
All of the tests pass. Normally, I would make a checkin here, but I’ve broken the build. No, the tests didn’t catch it.
The problem is in
Everything::initEverything(). The code that
creates a nodebase object actually creates a subtype of nodebase,
depending on the database used. The code for that is in
The problem is that I don’t want those modules there. I want them to
The best solution I can think of at the moment is to change
Everything::NodeBase to contain a storage object of
the proper subclass. I don’t have much confidence in the tests now, as they
didn’t catch this change. However, I’m making this change to make it easier
to write better (and probably smaller) tests that run against a real
database, so I’ll probably catch most of the bugs when I start that
I need to change
instantiate a normal nodebase object while passing the name of the storage
database to the nodebase constructor. There, I can create a new storage
object and store it as an attribute of the nodebase object.
Then I think I can just proxy all calls to the appropriate methods there. Maybe not. It’s worth testing though.
The tests fail, mostly because of the change to inheritance. That’s okay. (The tests aren’t great either; they’re the old style. Yuck.)
With a little bit of change, they run, but start to fail. Why?
Everything::DB objects lack the node cache parameter. I can
The rest of the tests are fairly easy to port. I added a new
object that wraps
Everything::DB::mysql and made most of the mock
object calls to the SQL-using functions use that
instead. All 223 tests pass.
The tests really need a rewrite, but I’ll do that later.
In the entire test suite, the
Everything module tests fail.
I believe it’s because of the change to
What’s the secret? That change.
After fixing up the tests, everything passes again in that test file and the entire suite.
That’s a good day. The diff will be huge, but that’s fine. This is checkin #880.
What’s next? There are no tests for the MySQL and the PostgreSQL code.
Everything::DB needs its own tests. The tests for
Everything::NodeBase need porting to the new style.
Even better, now I can make
(or whatever I call it) and remove lots of conditional code, letting
polymorphism take care of that instead. I can also switch the database used
while retaining most of the normal nodebase just by changing the
storage attribute of the nodebase object.