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.

Improving Node Metadata

What should I do for day 5? I could find all of the SQL and factor that elsewhere. I think instead I’ll make sure that all of the nodes are present in .pm files and that they all have inheritance set up correctly as well as metadata for their necessary tables. How long will that take? Unclear.

Looking at my list of inheritance (I keep these things), there are 16 unclassed nodes. It may be worth considering how to generate modules for these automatically when the server starts up, but the simplest thing for now is to add them. (Of course, I’ll probably forget to update the MANIFEST and to run the add script in scripts/, but I’m typing this now so I will have even less excuse.)

Each node class has the same format and needs the same type of test. That’s pretty easy. theme is the closest, so I can copy the code and the test and modify it all as I go.

It turned out that my list of inheritance was not quite right, but overwriting a file in source control isn’t awful (svk revert)…. The whole process took about 15 minutes. All tests pass. Now it’s time to add these files.

  $ for i in lib/Everything/Node/*.pm; do scripts/add_text_file.sh $i; done
  $ for i in t/Node/*.t; do scripts/add_text_file.sh $i; done

Updating the MANIFEST is a bit more tedious. I just followed the output of those two commands and added files manually. Zzzz… it would be nice to have a MANIFEST updating tool that showed you a diff between the files it has and the files in your working directory now, so you could add or remove things that way.

After adding everything, how does the complete test suite look? So far, good. That makes 54 test files with 1755 tests. That’s nice, though not comprehensive. This is checkin #835.

What’s next? Making each node have a dbtables() method that returns, in order of specificity, the tables required to work with an instance of each node. This will probably change greatly in the future (especially when pulling the database access out of these nodes), but it’s a small step forward for greater refactorings later. (It’s probably worth considering associating the specific column names with the table names too, but nothing will use this for now.)

All of the dbtable information is in the XML files in ecore, specifically the sqltable field. I just happen to have that here….

Adding this information to the tests is reasonably simple. Here’s the extra test for container:

  can_ok( $module, 'dbtables' );
  my @tables = $module->dbtables();
  is_deeply( \@tables, [qw( container node )],
      'dbtables() should return node tables' );

Halfway through this process, it’s fairly boring. There’s a lot of metadata here I wish I could specify somewhere and then use to generate this information. That’s a sign of weakness in the metamodel here. I’m not sure how best to fix this, but something like the Jifty::DBI model (or perhaps DBIx::Class does this well — I haven’t tried to work with it yet) of specifying the database schema in code and using that to generate the model classes will make this more satisfying.

Generating the appropriate tests would be nice too…

It would be nice if this process could also generate the full documentation, so that users don’t have to dive around in the entire inheritance hierarchy just to find information about the methods on a node with a class more than two levels deep. Hopefully Perl 6 POD will fix that, too.

The worst model so far is that of user, which inherits from setting (a has-a relationship, of course) and uses the user and document tables as well. The latter should be another has-a relationship. Of course, there’s nothing in the Everything Engine right now that supports aggregate relationships. That’s one of my goals of refactoring here.

After editing all 28 node classes and their tests, everything still passes and there are now 55 more tests in the world. (Why 55? I found and deleted a duplicate test in the nodegroup test.) Hooray. That’s checkin #836.

I also have the start of an idea on how to make it much easier to define and create new nodetypes. I think I can reuse the nodemethod scheme as well.

While I reviewed the diff, I noticed the awful, horrible use of UNIVERSAL::isa() and UNIVERSAL::can() as functions, not methods. I now maintain two modules on the CPAN intended to correct this abuse, so I might as well spend my last few minutes of the day fixing the code.

  $ vi $(grep -rl `UNIVERSAL::`)

It can be a little bit tricky getting this right, depending on what kind of data you expect, but I rely on the test suite exercising the code paths appropriately. After fixing one typo and two spots where the API allows either a node_id or an object, everything passes again. Because this code uses reftype() from Scalar::Util, I’ve marked a dependency in Build.PL on the oldest version I could find that included that function. Here’s checkin #837 and the end of another day.

(Now I wonder if overloading node stringification and numification to provide their IDs and titles is a bad idea. Maybe only numification is necessary.)