LinLog

As a brief follow-up to my last post, I had a problem trying to remote into my newly installed Ubuntu box today.  As I mentioned in that post, I'm using xRDP for my remote GUI access and I'm tunneling it over SSH.  When I connected to xRDP, I logged in and was greeted by the Ubuntu lock screen for my running session.  So I entered my password, hit "enter", and waited.  And waited.  And waited some more.  My session just never unlocked.

OK, so apparently xRPD is freaking out.  I already have a an SSH session open, so I'll just restart the service and maybe that will clear up the problem.  So I ran service xrdp restart and it told me:

Failed to add /run/systemd/ask-password to directory watch: No space left on device

That didn't seem right.  I had plenty of disk space and plenty of RAM - what could possibly be out of space?  So naturally I asked the internet, which led me to this Stack Exchange post.  Turns out that CrashPlan has a habit of eating up inotify watchers, which is what I was actually out of.  Stopping the CrashPlan service fixed the problem.  Fortunately, I don't need CrashPlan anymore, so that shouldn't be a problem again.

I finally decided to do it - reinstall my home media server.  I switched it from Windows 8.1 to Ubuntu 17.10.

The reinstall

Believe it or not, this is actually a big thing for me.  Fifteen years ago, it would have been par for the course.  In those days, I didn't have a kid or a house, so rebuilding my primary workstation from scratch every few months was just good fun.  But these days...not so much.  Now I have a house and a kid and career focus.  Installing the Linux distribution of the week isn't fun anymore - it's just extra time I could be spending on more important and enjoyable things.

But, in a fit of optimism, I decided to give it a shot.  After all, modern Linux distributions are pretty reliable, right?  I'll just boot up and Ubuntu DVD, it'll run the installer for 15 minutes or so, and I'll have a working system.  Right?

Well...not quite.  Turns out Ubuntu didn't like my disk setup.  Things went fine until it tried to install Grub and failed miserably.  I'm not sure why - the error message the installer put up was uninformative.  I assume it had something to do with the fact that I was installing to /dev/sdb and /dev/sda had an old Windows install on it.  After a couple of tries, I decided to just crack open the case and swap the SATA cables, making my target drive /dev/sda, and call it a day.  That did the trick and Ubuntu installed cleanly.  I did have to update my BIOS settings before it would boot (apparently the BIOS didn't automatically detect the change of drives), but it worked fine.

The only real problem I had was that apparently Wayland doesn't work properly on my system.  I tried several times to log into the defaut GNOME session and after a minute or two, system load spiked to the point that the GUI and even SSH sessions became unresponsive and eventually the GUI just died.  And I mean died - it didn't even kick me back to the GDM login screen.

I suspect the problem is my system's archaic video card - an integrated Intel card from the 2010 era which I have absolutely no intention of ever upgrading.  I mean, it apparently wasn't good enough to run Windows 10, so I wouldn't be surprised if Wayland had problems with it.  But in any case, Ubuntu still supports X.org and switching to the GNOME X.org session worked just fine.  I don't really intend to use the GUI on this system anyway, so it's not a big deal.

The restore

Once I got Ubuntu installed, it was on to step two of the process: getting my data drive set up.  It's a 1TB drive that used to serve as the Windows install drive.  It was divided into a couple of partitions, both formatted as NTFS.  Since I'm switching to Linux and really just wanted one big data drive, this was a sub-optimal setup.  Therefore I decided to just blow away the partition table and restore from a backup

I currently use CrashPlan to back up this computer.  It's set to back up to both the cloud and a local USB hard drive.  So my plan was to repartition the disk, install CrashPlan, and restore from the local hard drive.

This was fairly easy.  Installing the CrashPlan client was the first task.  There's no .deb package, but rather a .tgz that contains an installer script.  It was actually pretty painless - just kick off the script and wait.  It even installs its own copy of the JRE that doesn't conflict with the system version.  Nice!

Next was actually restoring the data.  Fortunately, CrashPlan has their process for restoring from a USB drive well documented, so there wasn't much to figure out.  The process of connecting a particular backup dataset on the drive to the CrashPlan client was slightly confusing because the backup interface (as far as I can remember) doesn't really make it obvious that a USB drive can have more than one dataset.  But it just boils down to picking the right directory, which is easy when you only have one choice.

The only surprise I ran into was that running the restore took a really long time (essentially all day) and created some duplicate data.  My data drive contained several symlinks to other directories on the same drive and CrashPlan apparently doesn't handle that well - I ended up with two directories that had identical content.  I'm not sure whether this was a result of having symlinks at all, or if it was just moving from Windows symlinks to Linux symlinks.  In any case, it's slightly inconvenient, but not a big deal.

Other services

While the restore ran, I started setting up the system.  Really, there were only a handful of packages cared about installing.  Unfortunately for me, most of them are proprietary and therefore not in the APT repository.  But the good news is that most of them were pretty easy to set up.

The first order of business, since this box is primarily a media server, was setting up Plex.  This turned out to be as simple as installing the .deb package and firing up the UI to do the configuration.  From there, I moved on to installing Subsonic.  This was only marginally more difficult, as I also had to install the OpenJDK JRE to get it to run.  I also followed the instructions here to make the service run as a user other than root, which seemed like not such a great idea.

The only thing I wanted to install that I couldn't get to work was TeamViewer.  But this wasn't a deal-breaker, though, because the only reason I was using TeamViewer under Windows was because I was running the Windows 8.1 Home and was too cheap to pay for the upgrade to Professional just to get the RDP server.  But since this is Ubuntu, there are other options.  Obviously SSH is the tool of choice for command-line access and file transfers.  For remote GUI access, I tried several variations on VNC, but it eventually became clear that xRDP was the best solution for me.  It's not quite as straight-forward to get working, but this guide provides a nice step-by-step walk through.  There are also a few caveats to using it, like the fact that the same user can't be logged in both locally and remotely, but those weren't big issues for my use case.

Results

For the most part, the transition has gone pretty smoothly.  The setup didn't go quite as smoothly as it could have been, but it wasn't too bad.  In any event, what I really cared about was getting Plex and Subsonic up and running and that was pretty painless.  So I'm back to where I was originally, but on an up-to-date operating system, which is all I really wanted.

The first part of my LnBlog refactoring is kind of a large one: changing the publication logic.  I'll start by giving an overview of the old design, discussing the new design, and then looking at some of the actual project data.

History

LnBlog is a very old system.  I started it back in 2005 and did most of the work on it over the next two years.  It was very much an educational project - it was my first PHP project (in PHP 4, no less) and only my third web-based project.  So I really didn't know what I was doing.

As you might expect, the original design was very simple.  In the first version, "publishing an entry" really just meant creating a directory and writing a few files to the disk.  As the system grew more features, more and more steps were added to that process.  The result was a tangle of code where all of the logic lived in the "controller" (originally it was a server-page) with some of the actual persistence logic encapsulated in the domain objects.  So, roughly speaking, the BlogEntry object knew how to write it's metadata file to the appropriate location, but it didn't know how to properly handle uploaded files, notifications, or really anything else.  

Originally, LnBlog used a server-page model, with each URL being a separate file and code being shared by including common configuration files and function libraries all over the place.  Shortly prior to starting this project, I had consolidated the logic from all those pages into a single, massive WebPages class, with one public method for each endpoint - a monolithic controller class, for all intents and purposes.  This still isn't great, but it gave me a common entry point to funnel requests through, which means I can better control common setup code, handle routing more easily, and generally not have the pain of dealing with a dozen or more endpoints.

Anyway, this WebPages class served up and edited entries by directly manipulating domain objects such as BlogEntry, Blog, BlogComment, etc.  The domain objects knew how to save themselves to disk, and a few other things, but the majority of the logic was right in the WebPages class.  This worked fine for the main use-case of creating an entry from the "edit entry" page, but it made things very awkward for the less-used locations, such as the Metaweblog API endpoint or scheduled publication of drafts.

Furthermore, the publication logic in the WebPages class was very hairy.  All types of publication flowed through a single endpoint and used a common code path.  So there were flags and conditions all over the place, and it was very difficult to tell what needed to be updated when making changes.  Bugs were rampant and since there was no test automation to speak of, testing any changes was extremely laborious and error prone.

The New Design

There were two main goals for this refactoring:

1. Create a new Publisher class to encapsulate the publication logic.  The idea was to have a single entity that is responsible for managing the publication state of blog entries.  Given a BlogEntry object, it would know how to publish it as a regular entry or a static article, unpublish it, handle updating or deleting it, etc.  This would give us a single entity that could own all the steps involved in publishing.
2. Create unit tests around the publication process.  The logic around the entire process was more complicated than you'd think and the old code was poorly structured, so it broke with disturbing regularity.  Hence I wanted some automated checking to reduce the risk of bugs.

So the design is actually pretty straight-forward: create a "publisher" class, give it operations for each of the things we do with blog entries (create, update, delete, etc.), copy the existing logic for those cases into the corresponding methods, update the endpoints to use this new class, and call it a day.  

So it was mostly just a reorganization - there wasn't any significant new logic that needed to be written.  Simple, right?  What could go wrong?

Results

While I was happy with the result, this project turned out to be a much larger undertaking than I'd assumed.  I knew it was going to be a relatively large task, but I was off by a factor of more than two.  Below is a table summarizing the project statistics and comparing them to the original estimates (from PSP data I captured using Process Dashboard). 

When I originally did the conceptual design and estimate, I had planned for relatively modest changes to the Article, BlogEntry, and WebPages classes and the creators.php file.  I also planned for new Publisher and BlogUpdater classes, as well as associated tests and some tests for the WebPages class.  This came to 29 hours and 946 new or changed lines of code across nine source files.  Definitely a big job when you consider I'm working in increments of two hours or less per day, whenever I get around to it.

In actuality, the changes were much larger in scope.  I ended up changing 27 additional files I hadn't considered, and ended up creating two other new utility classes (although I did ditch the BlogUpdater class - I no longer even remember what it was supposed to do).  The resulting 3138 lines of code took me 64 hours spread over five months.

Effects of Testing

I did test-driven development when working on this project.  I've found TDD to be useful in the past and it was very helpful to me here.  It was also very effective in meeting my second goal of building a test suite around the publication logic.  PHPUnit reports statement coverage for the Publisher tests at 97.52% and close to 100% coverage for the tested methods in the WebPages class (I only wrote tests for the endpoint that handles creating and publishing entries).

More importantly, using TDD also helped me to untangle the logic of the publication process.  And it turns out there was actually a lot to it.  I ended up generating about 2000 lines of test code over the course of this project.  It turns out that the design and unit test phase occupied 65% of the total project time - about 41 hours.  Having a comprehensive test suite was immensely helpful when I was rebuilding the publication logic across weeks and months.  It allowed me to have an easy check on my changes without having to load all of the code I worked on three weeks ago back into my brain.

Sadly, the code was not such that I could easily write tests against the existing code.  In fact, many of the additional changes came from having to break dependencies in the existing code to make it possible to unit test.  Luckily, most of them were not hard to break, e.g. using an existing file system abstraction layer, but the work still adds up.  It would have been very nice to have an existing test suite to prevent regressions in the rewrite.  Unfortunately, even integration tests would have been awkward, and even if I could have written them, it would have been very hard to get the level f coverage I'd need to be confident in the refactor.

Conclusion

In terms of the results, this project worked out pretty well.  It didn't really go according to plan, but I got what I was looking for in the end - a better publication design and a good test suite.  However, it was a long, slow slog.  Maybe that was too big a slice of work to do all at once.  Perhaps a more iterative approach could have kept things moving at a reasonable pace.  I'll have to try that on the next big project.

This evening I was reading a post on Jason Gorman's blog about fluent assertions in unit tests and it made me smile.  Jason was updating some slides for a workshop and decided to illustrate fluent assertions by putting the "classical" assertion right next to the fluent version.  After doing that, he started to doubt the text on the slide that claimed the fluent version is easier to read than the classical one.

That made me feel good, because I've often wondered the same thing.  I've heard the the assertion that fluent assertions are easier to read many times - for instance, they said that in a Certified Scrum Developer class I took last year.  Apparently that's just "the direction the industry is going."  I've followed Jason's blog for a while and he seems like a pretty smart guy, so seeing him doubt the received wisdom gives me a little validation that it's not just me.  

Fluent assertions are...sort of fine, I guess.  I mean, they work.  They're not terrible.  I never really thought they were any easier to read than the old-fashioned assertions, though.  

From what I can tell, the claim is generally that fluent interfaces are easier to read because they naturally read more like a sentence.  And that's definitely true - they undeniably read more like a sentence.  Just look at some of Jason's examples (classical above, fluent below):

Assert.AreEqual(50, 50);
Assert.That(50, Is.EqualTo(50));

Assert.IsTrue(true);
Assert.That(true);

Assert.AreSame(payer, payee);
Assert.That(payer, Is.Not.SameAs(payee));

Assert.Contains(1, new ArrayList() {1, 2, 3});
Assert.That(new ArrayList() {1, 2, 3}, Contains.Item(1));

It's undeniable that "assert that 50 is equal to 50" is much closer to a real sentence than "assert are equal of 50 and 50" (or however you would read the classical version).  However, it would behoove us to remember that we're reading code here, not English prose.  We can't just assume that making your code read like an English sentence makes it more readable.  They're different things.

My theory is that we tend to think differently when we're looking at code than we do when we're reading text.  The fluent version might look closer to a sentence, but that's really just on the surface.  It's only more natural if you mentally strip out all the punctuation.  But that's not how we read code, because in code you can't just strip out the punctuation.  The punctuation is important.  Likewise, the order of things is important, and not necessarily the same as in English.

When I look at the fluent assertion, I don't just see "assert that 50 is equal to 50", I see all the pieces.  I mentally separate the "assert" from the "that", because "that" is a method, which usually means it contains the more immediately applicable information.  Likewise with "is equal to".  And, of course, I recognize Is.EqualTo(50) as a method call, which means that I have to mentally substitute the result of that into the second parameter.  But wait a minute - equal to what?  There's only one parameter to EqualTo, which doesn't make any sense.  Oh, wait, that's NUnit magic.  So we pass 50 and the result of EqualTo to That...what the heck does "that" mean anyway?  Wait - magic again.  We have to read that backwards.  So the actual assertion comes at the end and the "that" just ties things together.  OK, I've got it now.  So...what were we testing again?

OK, maybe that's a little silly, but you get the idea.  The point is that code is written in a certain way, and it's not the way English is written.  When you're reading code, you get into a certain mindset and you can't easily just switch to a different one because the next line just happens to call a fluent interface.  The old-fashioned Assert.AreEqual(50, 50) might not be sexy or natural looking, but it's short, simple, and perfectly clear to any developer worth his salt.  Why switch to an interface that's wordier and less aligned with how the rest of our code is written?  If it ain't broke, don't fix it.  

Today, we're going to talk a little about design and refactoring.  As a case-study, we're going to use a little blogging application called LnBlog.  You probably haven't heard of it - it's not very popular.  However, you have used it, at least peripherally, because it's running this site.  And you also have at least a passing familiarity with the author of that application, because it's me. 

Motivation

Software is an "interesting" field.  The cool new technologies, frameworks, and languages get all the press and they're what everybody wants to work with.  But let's be honest: it's generally not what makes the money.  I mean, how could it be?  It just came out last week!

No, if you have the good fortune to work on a grown-up, profitable product, it's almost certainly going to be the "old and busted" tech.  It might not be COBOL or Fortran, but it's almost certainly "legacy code".  It might be C++ or Java or, in our case, PHP, but it's probably old, poorly organized, lacking unit tests, and generally hard to work with.

I work on such a product for my day job.  It's a 10-year-old PHP codebase, written in an old-fashioned procedural style.  There are no unit tests for the old code, and you couldn't really write them even if you wanted to.  Sure, there's a newer part with proper design, tests, etc., but the old code is the kind of stuff that "works for the most part", but everybody is afraid to touch it because it's so brittle and tightly coupled that God alone knows what will break when you make a change.

This also applies to LnBlog.  It was my very first PHP application.  I started it way back in early 2005, in the bad old days of PHP 4.  Over the next two or three years, I managed to turn it into something that was relatively functional and full-featured.  And for the last ten years or so, I've managed to keep it working.

Of course, it hasn't gotten a whole lot of love in that time.  I've been busy and, for the most part, it worked and was "good enough".  However, I occasionally need to fix bugs or want to add features, and doing that is a truly painful process.  So I would very much like to alleviate that pain.

The Issue

Let me be honest: I didn't really know what I was doing when I wrote LnBlog.  I was about four years out of school and had only been coding for about six or seven years total.  And I was working mostly in Visual Basic 6 at the time, which just barely counts.  It was also only my third web-based project, and the first two were written in classic ASP and VBScript, which also just barely counts.

As a result, it contains a lot of questionable design decisions and overly-complicated algorithms.  The code is largely procedural, kind of buggy, and makes poor use of abstraction.  So, in short, it's not great.

But, in fairness to myself, I've seen worse.  In fact, I've seen a lot worse.  It does have a class hierarchy for the domain objects (though it's a naive design), an abstraction layer for data access (though it's inconsistently used), and a templating system for separating markup from domain logic (though the templates are an ungodly mess).  And it's not like I had a role model or mentor to guide me through this - I was figuring out what worked on my own.  So while it's not great, I think it's actually surprisingly good given the circumstances under which it was built.

The Goal - Make the Code "Good"

So I want to make LnBlog better.  I've thought about rewriting it, but decided that I wouldn't be doing myself any favors by going that route.  I also hold no illusions of a grand re-architecture that will fix all the problems and be a shining beacon of design perfection.  Rather, I have a relatively modest list of new features and bug fixes, and I just want to make the code good enough that I can make changes easily when I need to and be reasonably confident that I'm not breaking things.  In other words, I want to do a true refactoring.

If you haven't read Martin Fowler's book, the word "refactoring" is not a synonym for "changing code" or "rewriting code".  Rather, it has a very specific meaning: improving the internal design of code without changing the external behavior.  In other words, all you do is make the code easier to work with - you don't change what it does in any way.  This is why people like Bob Martin tell you that "refactor X" should never be an item in your Scrum backlog.  It is purely a design and "code cleanliness" activity, not a "feature" you can deliver.

So my goal with LnBlog is to gradually reshape it into what it should have been in the first place.  This is partially to make changing it easier in the future.  But more importantly, it's a professional development goal, an exercise in code craftsmanship.  As I mentioned above, I've worked professionally on many systems that are even more messed up than LnBlog.  So this is a study in how to approach refactoring a system.  

And So It Begins...

My intention is to write a number of articles describing this process.  I've already completed the first step, which is rewriting some of the persistence and publication logic.  I'm using the PSP to track my planned and actual performance, so I'll have some actual data to use in my discussion of that process.  Hint: so far, the two are very different.

With any luck, this project will enable me to draw some useful or interesting conclusions about good and bad ways to approach reworking legacy systems.  Maybe it will also enlighten some other people along the way.  And if nothing else, I should at least get a better codebase out of it.