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 (
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
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
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
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
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
scheme as well.
While I reviewed the diff, I noticed the awful, horrible use of
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
node_id or an object, everything passes again. Because this
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
(Now I wonder if overloading node stringification and numification to provide their IDs and titles is a bad idea. Maybe only numification is necessary.)