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 Everything::NodeBase.

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 Everything::Storage a role provided by a more specific role implementation, then create a role-decorated class for each usable storage engine. First, this approach.)

I copied Everything::NodeBase into the new file, then deleted everything except the methods I want to keep. Then I added @EXPORT to 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 Everything::NodeBase::mysql and Everything::NodeBase::pg.

The problem is that I don’t want those modules there. I want them to subclass Everything::DB.

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

I need to change Everything::initEverything() to 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 add that.

The rest of the tests are fairly easy to port. I added a new Test::MockObject::Extends object that wraps Everything::DB::mysql and made most of the mock object calls to the SQL-using functions use that $mock_storage 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 initEverything(). 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 Everything::NodeBase::Workspaced (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.