dlowe WFH

When to Forget

2010-08-26T00:46:00.001-07:00

It is well-known that we programmers fill our brains with a lot of complex details while we're working on a problem in code. Some time later, due to our sadly finite brains, we go through the process of forgetting most of these details. Paul Graham, in his essay "Holding a Program in One's Head" summarizes this process nicely: "You can start to treat parts as black boxes once you feel confident you've fully explored them."

This forgetting is important! Software is complicated, and trying to hold all of the details of even a modest body of code will easily outstrip any programmer's mental capacity. We must forget things! But when? When is it safe to forget the details? When is it reasonable to feel confident you've fully explored them?

In my career I've worked with programmers who coped with this problem in various ways. Some were strictly FIFO (with various queue sizes, nudge-nudge wink-wink say no more). Some seemed to forget as soon as they checked the code in, others after it was merged to trunk, or tagged as a release candidate, or passed testing.

I think this is an important question to think about. When a defect is found (or a change is requested -- I'll treat these as the same from now on), a programmer with the implementation details in mind will be able to react faster and with more confidence than one who has to re-learn them. It's something like the difference between reading a page which is in RAM vs. reading a page which has been swapped out to disk: a couple orders of magnitude more work.

I'll go ahead and be opinionated here: the right time to start forgetting about code is after it's in production. There is a qualitative difference between the production release and everything that comes before that point, which is that nothing else actually matters. The real-world behavior of the software is the software, from the point of view of everyone who isn't a programmer. Nobody else cares that it worked in your sandbox!

Then, too, there's this rough graph I completely made up:

Briefly: defects in code are discovered throughout the software development process. But shortly after release (for some fairly variable definition of "shortly") there is a very sharp drop-off in defects found with new code. Until that sharp drop-off, a good programmer should have all of the pertinent details in mind, so that coming up with a fix doesn't involve a lot of head-scratching "what, wait, how does this work again?"

It's worth talking about that hand-wavey "shortly" for a sec. I've spent my entire career writing software which deployed to internal servers under crushing load; for me, shortly has always been less than 24 hours. I know that other types of software and other types of deployments have very different values of "shortly". I don't think that matters much -- the important point is that long-standing issues are vastly, vastly less common than ones which are discovered when the software first leaves the nest.

Flipping this whole thought around: you've probably seen a graph like this, showing how the cost to fix a defect rises drastically the longer the defect resides in the system:

This is typically (and reasonably) used to argue for better and earlier testing. But I've got a hunch, which I can only substantiate with my own limited experience: this graph is a lot flatter and less frightening in organizations which do rapid or continuous deployments. Partly because such organizations must (by definition) have scrubbed most of the time and effort from the deployment process. But also because the programmers don't have to re-learn all of the details of code written weeks or months (or years, yagh!) earlier.

Yes, in a perfect universe, we'd have all of the details of our software in mind forever. And learning the intricacies of other people's code would be easy. And defects would always be caught in testing and never in production. Here in reality: strive to keep the details in mind until after the code is released.

And if that's hard or impossible to do with your code, ask yourself: why? In my next post, I will go over a few specific bad practices I've encountered that work against this effort, and a couple of concrete things that can help.

The No-Cry Software Development Methodology

2009-09-16T00:32:00.000-07:00

The programming section at the bookstore is jammed with prescriptive "methodology" books like these:

"Continuous Integration: Improving Software Quality and Reducing Risk"
"Rapid Development: Taming Wild Software Schedules"
"Implementing Lean Software Development: From Concept to Cash"

Likewise the parenting section:

"The No-Cry Potty Training Solution"
"The Attachment Connection: Parenting a Secure & Confident Child Using the Science of Attachment Theory"
"The Successful Child: What Parents Can Do to Help Kids Turn Out Well"

I want to talk about two such books that I didn't write:

At my work we got by for a very long time with an ad hoc release process, wherein a release plan -- the list of operational steps required to make the release -- was one of the unique products of each release. This worked for a long time. About six months ago, though, we started tackling hard problems in badly bitrotted components, and suddenly we had a string of awful releases. Every release, something went blooey, either because of a mistake in the release plan, or a mistake executing its unfamiliar contents, or a mistake in the code which required (often middle of the night) manual intervention and cowboy heroics by operations and engineering.

This led to my personal least-favorite type of meeting, the post-mortem (kill me!). So, to prevent future post-mortem meetings, we had secret technical cabal post-post-mortem meetings, in which we thrashed with the problem in a somewhat more productive manner. Ultimately (after a few failed experiments and a bunch more lousy releases) we settled on a simple change: we'd have exactly one release plan, re-used for every single release, and we'd code to match the release plan rather than write the release plan to match the code.

A few months later: the operational team has scripted every aspect of the release process. Human QA had formalized testing of the release process. Engineering had created automated tests to cover the basic required release behavior. And though we continued to hack away at the (historically) most brittle components in every release, we're back to weekly and almost completely uneventful releases.

A certain type of person seems to get to this point in their problem solving and suddenly think: "I should write a book about release methodology. I'll call it 'The No Post-Mortem Meeting Release Solution'! It'll sell like hotcakes, *everyone* hates post-mortem meetings!"

In my other life, as a parent, we recently went through a spell of awful fighting between the kids. Screaming, pushing, crying, name-calling, etc. Totally normal stuff for young kids, but still really unpleasant. After some thought and a lot of experimentation and so on, one of the tactics that worked really well was to start a cooking project when things were getting tense, and invite one or both of the kids to help. I started baking pies like crazy. And it really helps!

A few months later, it is not uncommon for the kids to be the ones to recognize their own boredom and suggest a cooking project. We eat pie every other day or so, which would make it well worthwhile even if the kids still fought incessantly... and while I can't say that fighting has been completely eliminated, it's better.

Again some people seem to get to this point and think: "Fantastic! I will now write and publish 'The Shoo-Fly Pie and Apply Pan Dowdy Cure For Sibling Rivalry'!"

In both cases, the new "methodology" arose when creative people, not overly constrained by conventions, traditions or rules, innovated specific tools to address specific problems.

So what about those books? They seem harmless enough -- someone came up with a solution to their specific problem and thought it might help others in similar situations. It strikes me as silliness to think that one can create a universal methodology for something as complex as potty training an individual toddler, or releasing any arbitrary piece of software. But I do think it's great to draw on the experiences of others when trying to solve a problem.

However, there is no value in a methodology if it does not solve a real problem. On the contrary: any tactic, strategy, methodology or philosophy that you adopt which is not solving a specific problem that *you* are having *right now* is a problem in and of itself. YAGNI! As with bloated code, a bloated process adds inertia and inflexibility. The more bloated it gets, the *harder* it will be to solve the unique problems you're going to face -- for which there is no prescription!

Caricature Rational Programmer (CRP): "I found this memory leak and it's gonna pooch the release that's supposed to go out tomorrow so I'm filing a CRM and a ticket and stuff and we're gonna slip by at least a month."
Me: "Fix the leak and save the release."
CRP: "But we're Rational Unified! And IBM says if we don't go through the change committee our software will be late, over-budget, defect-ridden and incomprehensible!"

Of course, any methodology can be abused this way (it's just that today is international pick on Rational Unified Process day). And I see this sometimes in parents, too:

Caricature Attachment Parent (CAP): "Co-sleeping is making my spouse and I miserable, the baby kicks like a ninja all night and we're not getting any sleep."
Me: "Put the baby in a crib; you'll sleep better after a few nights."
CAP: "But we're Attachment Parents! And Dr. Sears says unless you co-sleep your kid will be stupid and insecure!"

Yeah, they're caricatures. But here's the thing: anyone who wasn't there when the original problem was solved can never truly grok why they're doing things this way. They do it out of faith and tradition, perhaps fear -- which can all-too-easily become religion and dogma.

Debugging and Science

2009-02-28T18:06:00.001-08:00

I just finished reading Richard Feynman's collection of autobiographical sketches, "Surely You're Joking, Mr. Feynman" which is a lot of fun. He was a first-class kookball.

At one point Feynman talks about the moment when he figures something out: a new theory or insight or solution that neatly explains some previously mysterious behavior. And I recognized some of the feeling he was talking about, surprisingly enough, from the feeling of recently rooting out the causes of a years-old mandelbug at work.

This got me thinking about debugging, and programming, and complexity, and finally about mysteries...

As a high school student I was drawn to science, especially chemistry. But at some point I got discouraged: it seemed that all the interesting questions had already been answered. Now I'm old enough to realize that's far from true, but at least in my high school, science was approached as a very long series of answers, rather than questions. Experiments were basically mechanical: the results were all known ahead of time. No surprises and no mysteries. In fact, if an experiment's results were surprising, it was because the student made an error.

At the same time, I was taking programming classes (not to mention coding at home). And though I didn't think of it this way at the time, mysteries were popping up all the time. For example: I wrote a really simple polygon painting program (in MacPascal!) in one of my first CS classes. While working on it I'd get these really strange failure cases -- like, draw three triangles in a row and then hover the mouse for three seconds in the lower right hand corner of the window, and half the screen turns black. Stuff like that. Mysterious! So of course I'd stay late at school to solve it, and in the end there was always an explanation, however silly.

I think that was a big part of the allure of programming for me: the way even a fairly simple set of instructions can behave in ways even their creator finds mysterious.

Now that I work professionally on this stuff, it's even better. The software I write is layered on top of a *lot* of software written by others, which is layered on top of still deeper software, which is layered on top of an operating system, also software. And it's all open source (except the stuff I work on directly, ahem... but anyway I have that source code) so I can actually explain *anything*, if I have the patience and skill to figure it out. Except that one time where it turned out to be random memory corruption in a bunk SIMM... but that wasn't fair. Anyway, the point is, this stuff is complicated! It's not nuclear physics, and the "laws of nature" one discovers while debugging a complicated system of software usually have no meaning outside that system. But still, there's this satisfaction: I solved it!

Anyway, putting two and two together: I love debugging complex software systems because the harder it is, the more satisfying it is to fully explain some behavior. If my high school science teachers had talked about, say, the protein folding problem, I might very well have ended up a theoretical chemist.

Vendetta

2008-06-23T21:05:00.000-07:00

Yesterday, my boss noticed that I have already (6 months into the job) accumulated at least a years' worth of future work in trac tickets under my name. And he helpfully suggested maybe we could reassign a few to someone else. So I helped with the triage, right up until we got to #178. He suggested we give it to Mr. Black, and at that moment my head started spinning around, green foam spraying out of my nose and mouth (this was over IM; I used the old-fashioned emoticon *@#$*$@#*%! to convey my feelings).

"THAT ONE'S MINE!" was the best I could do to justify my response.

Funny, that #178. I filed it myself, assigned it to myself with no milestone or schedule, and didn't cc anyone on the ticket. The comments tell a story: under some conditions, a seemingly simple operation may take several minutes to complete, here's a brief outline of how to reproduce, how to avoid, where to find the brain dead code, some notes on the character flaws that would lead someone to write code that stupid, and where to find the (retired) author should the need arize to brutally murder him. Though #178 is tagged as a defect, it's a completely internal defect -- only other developers will ever be harmed, and only under very limited circumstances. And worse, I figure it'll take 5-6 days of effort to fix. I know there's basically no chance, given the workload and time pressure, that it will end up on the official schedule.

So my boss, ever helpful, heard all that and suggested just resolving the ticket into the 'wontfix' state. And again, I bent berzerk.

Have you seen "Kill Bill"? Suggesting that I 'wontfix' #178 was like suggesting to Uma Thurman's character that she turn the other cheek. "What Would Jesus Do?" says my boss. Whack, there goes his arm. Suggesting that we assign it to Mr. Black was, I believe, the equivalent of saying to her: "Why don't you hire a real hit man to get Bill for you?" Arterial spray everywhere!

I know #178 isn't the only ticket in the world that has this personal revenge characteristic. I carried one around for 6 years at a previous job, titled "majorshano must die". Time estimate: 4 weeks. And it will haunt me to my grave. (maybe I still have a copy of that code around somewhere... if I ever have a few spare weeks...) People kept trying to close it on the basis that after 4 years, it was probably never going to be fixed. And I would violently reopen it, assigned to myself, and show it to new hires when they stumbled over the code. "We know this is a nightmare," the ticket reassures them, "but here's the history..."

Let me tell you about majorshano. I found it a few months into the job. Buried deep under layers of aging enterprise middleware framework and custom C code, I found an ancient (perl 4 era) hacked up version of 'majordomo', in a 90k file called 'majorshano.pl', which had been wired in via an embedded perl interpreter (and called back out into the C middleware via buggy, hand-written XS code). The only good that ever came of majorshano is that it inspired me to write this.

I've had other revenge tickets that actually got fixed, because they had customer-facing implications. And the satisfaction I felt when obliterating those nasty chunks of code from their obfuscated hidey-holes is the same I imagine the heroic samurai feels when he finally drives his katana into guts of the evil ninja who murdered his master-slash-father-figure, and watches the shock of recognition as the life drains out of his eyes. "Hello. My name is dlowe. You killed my father. Prepare to die."

At my current work, this led to the addition of a new ticket type: Vendetta. Now my superiors and peers know what they're dealing with when they look at #178. Want to make me foam at the mouth? Close it or gaffle it. Want to secure my fanatical loyalty? Put it on the schedule (and try to keep people out of the blast radius.)

I know I'm not alone. Whether they keep them in the official tracking system or not, I'm convinced every programmer on every serious project has at least one piece of code that makes them completely, irrationally, overwhelmingly angry. It's not professional, it's personal. And makes good stories... want to tell me about yours?

When perl is too fast

2008-02-19T23:37:00.000-08:00

The idea for this post has been brewing for a few months, but I was inspired by this post from David MacIver to get on it.

I like Perl. From a trendiness standpoint this falls somewhere between admitting that I like peanut-butter-and-pickle sandwiches and admitting that I like clubbing baby seals. I understand and can appreciate the common complaints: the language is too big, its syntax is baroque, it is write-only, TMTOWTDI is a bug rather than a feature. But I want to talk about something else that has been irking me.

I am a code quality freak. I like to write and work on good, readable, maintainable code. When I find bad code, I fix it. And I don't believe in blaming my tools, so I don't rail against Perl syntax -- I just write the best, clearest, simplest code I can.

I am also a speed freak. Micro-optimizing, benchmarking, profiling, shaving the milliseconds off things that already run fast, these are some of my favorite things to do. And I am more often surprised by how fast perl is than by how slow it can be.

Most of the time, these two inner freaks aren't at odds, and I'm able to do my job (make a moderately-sized, heavily-loaded piece of Perl software run really bloody fast) without compromising my quality standards. But several times recently, I have bumped into problems where Perl has pushed me to seriously compromise on code quality in order to get the performance I needed. The problem isn't that perl is slow. Not at all! On the contrary, some of the bits of perl are too damn fast relative to other approaches -- so fast that performance critical Perl code has effectively no choice. And in some cases they are the worst bits you can imagine, from the point of view of writing clean, readable code.

Let me jump right away into an example. Consider a trivial perl function that takes a single scalar argument and returns its square. I'd bet that anyone with any imperative programming experience can read it (though they might wonder about the 'shift'):

sub square {
    my $n = shift;
    return $n * $n;
}

There is nothing wrong with this code, and in any sane language the only way to improve its performance would be to inline it. But in Perl you can make it roughly 25% faster by writing it thus:

sub square {
    return $_[0] * $_[0];
}

The trick is using the scalar directly off the function's parameter stack (no copy is made). The code is brutally bad (and it gets way, way worse if there are multiple arguments or the function is more than 2 lines long) but from a performance standpoint it is "best practice" for critical functions in Perl. And that is actually a well enough known optimization that some coders apply it automatically, and it is moderately common in CPAN code.

Knuth's "Premature optimization is the root of all evil" is true in any language... but Perl isn't helping here.

Onto a more complex example. The codebase I'm working in is absolutely littered with blocks like this:

my $stuff = compute_50_megs_of_stuff();
if ($debug_flags{'stuff'}) {
    print Data::Dumper::Dumper($stuff);
}

There are a lot of paths to improving this code, but we really wanted a couple of "meta" features -- a flag to enable all other flags, and a flag to automatically timestamp every debugging print -- which pointed to getting all of the debug prints to go through a single point. Perl has a simple mechanism for adding new syntax, so I decided what I wanted was that the above code should be rewritten as:

my $stuff = compute_50_megs_of_stuff();
debug_stuff { Data::Dumper::Dumper($stuff) }

(Aside: not debug_stuff(Data::Dumper::Dumper($stuff)). That would generate a 50-megabyte string representation of $stuff, which would then be discarded 99.9999% of the time (i.e. when not debugging)).

So debug_stuff would look like this:

sub debug_stuff (&) {
    if ($debug_flags{'stuff'} or $debug_flags{'all'}) {
        my $string_generator = shift;
        my $eol = "\n";
        if ($debug_flags{'timestamp'}) {
            $eol = "[[[" . time() . "]]]\n";
        }
        print $string_generator->() . $eol;
    }
}

I determined through experimentation that we could afford the overhead of a function call for each debugging statement (there are a *lot* of them, some in very tight loops). Even that wasn't an easy decision, which is sadly telling. Function call overhead is the reason I keep bumping into "mega methods": where all possible inlining has been done, at the expense of all maintainability and reusability of the code...

I digress. Once implemented, system performance was unacceptable, and I had to scramble to figure out why. It turned out that it was not because of the function overhead, but because of the second-class syntax extension. Specifically, Perl syntax extension is sugar for higher-order functions with anonymous function arguments. These anonymous functions may be closures, which (it turns out) impose significant overhead. In a trivial example, an inline 'if' is almost 4x faster than an extension if the block isn't a closure (that's the function call overhead) but over 25x faster if the block is a closure. Again: in performance-critical code, where you're scrambling for every millisecond, you "can't" use domain-specific syntax (or any higher-order abstraction which relies on closures, for that matter) which is really unfortunate from the standpoint of quality.

I would rather see a slower perl, if that's what it took for me to get away with writing higher quality code. If extension syntax had the same performance characteristics as built-in syntax, I would have been able to get away with that. If creating and passing anonymous subroutines were the same regardless of whether they were closures, I'd be able to write higher-order code whenever appropriate...

Of course, I probably wouldn't be working in Perl at all. It wouldn't be fast enough.

Code Review: MySQLDAO.java

2007-10-17T11:05:00.001-07:00

(DRY, bottom-up programming)

The Code

Today's review is of a Java class called MySQLDAO.java:

MySQLDAO.java
... public class MySQLDAO implements DAO { public List<Part> fetchPartsBySinkId(long sinkId, long begin, long end) { StringBuffer query = new StringBuffer("select " + "part.name as name, part.id as id from part, " + "sink, sink_part where sink.id = ? and " + "sink_part.sink_id = sink.id and part.id = " + "sink_part.part_id group by part.name, part.id"); if (end > 0) { query.append(" limit ?,?"); } Connection connection = getPooledConnection(); PreparedStatement ps = connection.prepareStatement(query.toString()); ps.setLong(1, sinkId); if (end > 0) { ps.setLong(2, begin); ps.setLong(3, end - begin); } ResultSet rs = ps.executeQuery(); List<Part> parts = new ArrayList<Part>(); while (rs.next()) { Part part = new Part(); part.setName(rs.getString("name")); part.setId(rs.getLong("id")); parts.add(part); } return parts; } public List<Sink> fetchSinksNeedingPartId(long partId) { StringBuffer query = new StringBuffer("select " + "sink.id as id, count(part.id) as count from " + "sink, sink_part, part where part.id = ? and " + "sink_part.part_id = part.id and sink.id = " + "sink_part.sink_id group by sink.id"); Connection connection = getPooledConnection(); PreparedStatement ps = connection.prepareStatement(query.toString()); ps.setLong(1, partId); ResultSet rs = ps.executeQuery(); List<Sink> sinks = new ArrayList<Sink>(); while (rs.next()) { Sink sink = new Sink(); sink.setId(rs.getLong("id")); sink.setPartCount(rs.getLong("count")); sinks.add(sink); } return sinks; } ... 30 more of these fetch* methods.

MySQLDAO.java

...
public class MySQLDAO implements DAO {
  public List<Part> fetchPartsBySinkId(long sinkId,
    long begin, long end) {
    StringBuffer query = new StringBuffer("select "
      + "part.name as name, part.id as id from part, "
      + "sink, sink_part where sink.id = ? and "
      + "sink_part.sink_id = sink.id and part.id = "
      + "sink_part.part_id group by part.name, part.id");
    if (end > 0) {
      query.append(" limit ?,?");
    }
    Connection connection = getPooledConnection();
    PreparedStatement ps =
      connection.prepareStatement(query.toString());
    ps.setLong(1, sinkId);
    if (end > 0) {
      ps.setLong(2, begin);
      ps.setLong(3, end - begin);
    }
    ResultSet rs = ps.executeQuery();

    List<Part> parts = new ArrayList<Part>();
    while (rs.next()) {
      Part part = new Part();
      part.setName(rs.getString("name"));
      part.setId(rs.getLong("id"));
      parts.add(part);
    }
    return parts;
  }

  public List<Sink> fetchSinksNeedingPartId(long partId) {
    StringBuffer query = new StringBuffer("select "
     + "sink.id as id, count(part.id) as count from "
     + "sink, sink_part, part where part.id = ? and "
     + "sink_part.part_id = part.id and sink.id = "
     + "sink_part.sink_id group by sink.id");
    Connection connection = getPooledConnection();
    PreparedStatement ps =
      connection.prepareStatement(query.toString());
    ps.setLong(1, partId);

    ResultSet rs = ps.executeQuery();

    List<Sink> sinks = new ArrayList<Sink>();
    while (rs.next()) {
      Sink sink = new Sink();
      sink.setId(rs.getLong("id"));
      sink.setPartCount(rs.getLong("count"));
      sinks.add(sink);
    }
    return sinks;
  }
... 30 more of these fetch* methods.

This class is a side-effect of using some kind of semi-automatic SQL-generating ORM framework like Hibernate or entity beans in a big project. Once the complexity of what they need to fetch from the database outgrows what the framework is capable of auto-fetching, projects tend to end up with something like the above. I believe there are two major reasons: first, one of the nice things about such a framework is that it removes embedded SQL from the main-line code -- and embedded SQL in Java code is hideously ugly, as you can see above. Second, these frameworks are supposed to make your code portable between different databases -- so even if, in practice, we're committed to one particular database, you have to write the code in such a way that switching databases is conceivable (or else face the fact that the framework's complexity is probably not justified!)

The big problem I see with this code is how repetitive it has become: every single public method in here has the same shape: build query, execute query, iterate through the result set converting into collection of java objects, clean up and return the collection. This kind of structural repetition seems to be invisible to many, if not most, Java programmers. But it's just as bad as more obvious forms of repetition. Consider two situations where this repetition suddenly causes big headaches:

You really do have to port this application to Oracle. All of a sudden, it is apparent that the repetitive shape is actually database agnostic, because the strategy is to copy the entire MySQLDAO anS then only modify the actual embedded queries.
Certain critical parts of the application start running out of memory, and you realize that instantiating and returning potentially huge collections isn't scalable. Now it is apparent that the repetitive shape is also query agnostic, because the strategy is to make copies of the methods in which the only thing that changes is the control shape -- the query building and converting code are the same.

Either of these requirements will likely turn this code from a barely-maintainable mess into a completely unmaintainable mess, unless the underlying repetitiveness of the code is addressed.

How Can I Improve This Code?

I'll start by stating my goal: I want to remove the repetition.

My strategy is to decompose the code until the repetition is painfully obvious. In this case, I'll break out the "build query" and "convert into java objects" chunks:

MySQLDAO.java
... public class MySQLDAO implements DAO { private PreparedStatement preparePartsBySinkIdQuery( Connection connection, long sinkId, long begin, long end) { StringBuffer query = new StringBuffer("select " + "part.name as name, part.id as id from part, " + "sink, sink_part where sink.id = ? and " + "sink_part.sink_id = sink.id and part.id = " + "sink_part.part_id group by part.name, part.id"); if (end > 0) { query.append(" limit ?,?"); } PreparedStatement ps = connection.prepareStatement(query.toString()); ps.setLong(1, sinkId); if (end > 0) { ps.setLong(2, begin); ps.setLong(3, end - begin); } return ps; } private Part convertPartsBySinkIdResult(ResultSet rs) { Part part = new Part(); part.setName(rs.getString("name")); part.setId(rs.getLong("id")); return part; } public List<Part> fetchPartsBySinkId(long sinkId, long begin, long end) { Connection connection = getPooledConnection(); PreparedStatement ps = preparePartsBySinkIdQuery(connection, sinkId, begin, end); ResultSet rs = ps.executeQuery(); List<Part> parts = new ArrayList<Part>(); while (rs.next()) { parts.add(convertPartsBySinkIdResult(rs)); } return parts; } ... public List<Sink> fetchSinksNeedingPartId(long partId) { Connection connection = getPooledConnection(); PreparedStatement ps = prepareSinksNeedingPartIdQuery(connection, partId); ResultSet rs = ps.executeQuery(); List<Sink> sinks = new ArrayList<Sink>(); while (rs.next()) { sinks.add(convertSinksNeedingPartIdResult(rs)); } return sinks; } ...

MySQLDAO.java

...
public class MySQLDAO implements DAO {
  private PreparedStatement preparePartsBySinkIdQuery(
    Connection connection, long sinkId, long begin,
    long end) {
    StringBuffer query = new StringBuffer("select "
      + "part.name as name, part.id as id from part, "
      + "sink, sink_part where sink.id = ? and "
      + "sink_part.sink_id = sink.id and part.id = "
      + "sink_part.part_id group by part.name, part.id");
    if (end > 0) {
      query.append(" limit ?,?");
    }
    PreparedStatement ps =
      connection.prepareStatement(query.toString());
    ps.setLong(1, sinkId);
    if (end > 0) {
      ps.setLong(2, begin);
      ps.setLong(3, end - begin);
    }
    return ps;
  }
  private Part convertPartsBySinkIdResult(ResultSet rs) {
    Part part = new Part();
    part.setName(rs.getString("name"));
    part.setId(rs.getLong("id"));
    return part;
  }
  public List<Part> fetchPartsBySinkId(long sinkId,
    long begin, long end) {
    Connection connection = getPooledConnection();
    PreparedStatement ps =
      preparePartsBySinkIdQuery(connection, sinkId, begin,
        end);

    ResultSet rs = ps.executeQuery();

    List<Part> parts = new ArrayList<Part>();
    while (rs.next()) {
      parts.add(convertPartsBySinkIdResult(rs));
    }
    return parts;
  }
...
  public List<Sink> fetchSinksNeedingPartId(long partId) {
    Connection connection = getPooledConnection();
    PreparedStatement ps =
      prepareSinksNeedingPartIdQuery(connection, partId);

    ResultSet rs = ps.executeQuery();

    List<Sink> sinks = new ArrayList<Sink>();
    while (rs.next()) {
      sinks.add(convertSinksNeedingPartIdResult(rs));
    }
    return sinks;
  }
...

Two big observations about this revision. First, the prepare and convert methods form a logical pair, and should probably be wrapped together in a simple object which encapsulates all of the logic for working with this specific query: let's call this 'interface Query'. Second, the fetch*() methods are looking pretty damn... generic. What's the abstract version? Given an object which implements the Query interface, it should return a List of java objects. But with generics, I can do better: by parameterizing the Query interface by the type of object it returns and parameterizing the fetchList() method the same way, I get all of the type safety and none of the repetition.

At the same time, notice that once the specific queries have been factored out, the abstract fetching mechanism is actually independent of the database, so it can be moved out of the MySQL-specific code.

Finally, the MySQLDAO, with all of the generic DAO code factored out, should probably be renamed to MySQLQueryFactory.

Here's the new version:

DAO.java
... public class DAO { private static QueryFactory queryFactory = new MySQLQueryFactory(); public interface Query<T> { public PreparedStatement prepare(Connection connection); public T convert(ResultSet rs); } private <T> List<T> fetchList(Query<T> q) { Connection connection = getPooledConnection(); PreparedStatement ps = q.prepare(connection); ResultSet rs = ps.executeQuery(); List<T> data = new ArrayList<T>(); while (rs.next()) { data.add(q.convert(rs)); } return data; } public List<Part> fetchPartsById(long sinkId, long begin, long end) { return fetchList(queryFactory.partsById(sinkId, begin, end)); } public List<Sink> fetchSinksNeedingPart(long partId) { return fetchList(queryFactory.sinksNeedingPartId(partId)); } ...

DAO.java

...
public class DAO {
  private static QueryFactory queryFactory =
    new MySQLQueryFactory();

  public interface Query<T> {
    public PreparedStatement
      prepare(Connection connection);
    public T convert(ResultSet rs);
  }

  private <T> List<T> fetchList(Query<T> q) {
    Connection connection = getPooledConnection();
    PreparedStatement ps = q.prepare(connection);
    ResultSet rs = ps.executeQuery();

    List<T> data = new ArrayList<T>();
    while (rs.next()) {
      data.add(q.convert(rs));
    }
    return data;
  }

  public List<Part> fetchPartsById(long sinkId,
    long begin, long end) {
    return fetchList(queryFactory.partsById(sinkId, begin,
      end));
  }

  public List<Sink> fetchSinksNeedingPart(long partId) {
    return
      fetchList(queryFactory.sinksNeedingPartId(partId));
  }
...

MySQLQueryFactory.java
... public class MySQLQueryFactory implements QueryFactory { public DAO.Query partsById(final long sinkId, final long begin, final long end) { return new DAO.Query() { public PreparedStatement prepare(Connection c) { StringBuffer query = new StringBuffer("select " + "part.name as name, part.id as id from part, " + "sink, sink_part where sink.id = ? and " + "sink_part.sink_id = sink.id and part.id = " + "sink_part.part_id group by part.name, " + "part.id"); if (end > 0) { query.append(" limit ?,?"); } PreparedStatement ps = c.prepareStatement(query.toString()); ps.setLong(1, sinkId); if (end > 0) { ps.setLong(2, begin); ps.setLong(3, end); } return ps; } public Part convert(ResultSet rs) { Part p = new Part(); p.setId(rs.getLong("ID")); p.setString(rs.getString("NAME")); return p; } }; } ...

MySQLQueryFactory.java

...
public class MySQLQueryFactory implements QueryFactory {
  public DAO.Query partsById(final long sinkId,
    final long begin, final long end) {
    return new DAO.Query() {
      public PreparedStatement prepare(Connection c) {
        StringBuffer query = new StringBuffer("select "
          + "part.name as name, part.id as id from part, "
          + "sink, sink_part where sink.id = ? and "
          + "sink_part.sink_id = sink.id and part.id = "
          + "sink_part.part_id group by part.name, "
          + "part.id");
        if (end > 0) {
          query.append(" limit ?,?");
        }
        PreparedStatement ps =
          c.prepareStatement(query.toString());
        ps.setLong(1, sinkId);
        if (end > 0) {
          ps.setLong(2, begin);
          ps.setLong(3, end);
        }
        return ps;
      }

      public Part convert(ResultSet rs) {
        Part p = new Part();
        p.setId(rs.getLong("ID"));
        p.setString(rs.getString("NAME"));
        return p;
      }
    };
  }
...

Much better!

Back to my earlier examples:

Port to Oracle. Still a big job, and still probably starts with a big cut & paste hack. But there's less code, and a much higher proportion of that code is actually (potentially) database-specific.
Fetch an iterator instead of a list. Easy:

DAO.java
... public class DAO { ... private <T> Iterator<T> iterateList(final Query<T> q) { Connection connection = getPooledConnection(); PreparedStatement ps = q.prepare(connection); final ResultSet rs = ps.executeQuery(); return new Iterator<T>() { private T next; public boolean hasNext() { if (next == null) { if (! rs.next()) { return false; } next = q.convert(rs); } return true; } public T next() { if (! hasNext()) { throw new NoSuchElementException(); } T retval = next; next = null; return retval; } public void remove() { throw new UnsupportedOperationException(); } }; } ... public Iterator<Part> iteratePartsById(long sinkId) { return iterateList(queryFactory.partsByIdQuery(sinkId, 0, 0)); } ...

DAO.java

...
public class DAO {
...
  private <T> Iterator<T> iterateList(final Query<T> q) {
    Connection connection = getPooledConnection();
    PreparedStatement ps = q.prepare(connection);
    final ResultSet rs = ps.executeQuery();

    return new Iterator<T>() {
      private T next;

      public boolean hasNext() {
        if (next == null) {
          if (! rs.next()) {
            return false;
          }
          next = q.convert(rs);
        }
        return true;
      }
      public T next() {
        if (! hasNext()) {
          throw new NoSuchElementException();
        }
        T retval = next;
        next = null;
        return retval;
      }
      public void remove() {
        throw new UnsupportedOperationException();
      }
    };
  }
...
  public Iterator<Part> iteratePartsById(long sinkId) {
    return iterateList(queryFactory.partsByIdQuery(sinkId,
      0, 0));
  }
...

And in case this isn't obvious, iterateList() can be reused on any Query object. This brings me to bottom-up programming: identifying operations which are fundamental to your software, and making them part of the language. I've gone about it backwards in this case, by factoring the "bottom" out of an existing lump of code, but the effect is the same: by recognizing that this project is going to do a lot of database fetches, adding functionality to make this easier and less repetitive to the language itself makes development that much easier.

For what it's worth, I think this code can still be improved, by looking at it through the lens of cohesion. But this is enough for now.

Code Review: Constants.pm

2007-10-11T22:07:00.001-07:00

(cohesion, scope, laziness)

The Code

Today I took home a chunk of code to review. It's a chunk of code which I've run across, in one form or another, for 10 years now, and it drives me around the bend. In Perl: Constants.pm. In Java: Constants.java. In C/C++: constants.h.

Let's have a look at the perl module KitchenSink/Constants.pm:

$ ls -ln Constants.pm
-rw-r--r--   1 501  501  41387 Sep 17 2007 Constants.pm

First: holy Larry Wall, that's a big module! What the heck is going on in there? There can't really be 40k worth of constants in this project, can there? Yes, apparently there can. Okay, in keeping with my study strategy, how can I describe the problems with this module in precise terms, and how can I improve it?

A peek inside the file shows more or less what you'd expect:

Constants.pm
... our @EXPORT = qw( DEBUGLOG_LEVEL CC_STATUS_SUCCESS CC_STATUS_CCPROBLEM RET_SUCCESS RET_API_ERROR RET_INTERNAL_ERROR ... ); use constant DEBUGLOG_LEVEL => 2; use constant CC_STATUS_SUCCESS => 0; use constant CC_STATUS_CCPROBLEM => -1; use constant RET_SUCCESS => 0; use constant RET_API_ERROR => 1000; use constant RET_INTERNAL_ERROR => 2000; ...

Constants.pm

...
our @EXPORT = qw(
DEBUGLOG_LEVEL

CC_STATUS_SUCCESS
CC_STATUS_CCPROBLEM

RET_SUCCESS
RET_API_ERROR
RET_INTERNAL_ERROR
...
);

use constant DEBUGLOG_LEVEL => 2;

use constant CC_STATUS_SUCCESS => 0;
use constant CC_STATUS_CCPROBLEM => -1;

use constant RET_SUCCESS => 0;
use constant RET_API_ERROR => 1000;
use constant RET_INTERNAL_ERROR => 2000;
...

And the consumers of these values:

Log.pm
... sub log { my ($level, $message) = @_; if ($level >= DEBUGLOG_LEVEL) { print LOG $message; } } ...

CreditCard.pm
... if ($vrc == CC_STATUS_SUCCESS) { $total_charged += $amount; } elsif ($vrc == CC_STATUS_CCPROBLEM) { $total_failed += $amount; } ...

hundreds of files throughout the project
... $retval = someServiceMethod(); die KitchenSink::Error::describeRet($retval) unless $retval == RET_SUCCESS; ...

Way back at the dawn of time, when there were only a few thousand lines of code and a dozen or so constants, someone lumped them all together. It was a bad idea from the beginning, and nobody ever questioned this organization, so now there are a half million lines of code and almost a thousand constants, and it's a great stinky mess.

Let's look at cohesion -- how closely related are the things in this package? Highly cohesive modules are desirable. Well... everything in here is constant, so they're all the same, so the cohesion is very high! This counter-argument is absolutely true, but the code is still making me want to gouge out my eyeballs, so what's going on?

It turns out there are lots of kinds of cohesion. What we have here is a form of logical cohesion -- grouping into some logical "category". Check out this wikipedia page for a reasonable breakdown of different types of cohesion. In short, though, logical cohesion is among the worst kinds. It is all the more dangerous because it's possible to defend it as strongly cohesive.

How Can I Improve This Code?

As long as there's a file called Constants.pm, I know there's some poorly organized code. Consider the fictional modules VariablesBeginningWithG.pm and ArraysCreatedByDlowe.pm: You know just by looking at the names of these modules that the code inside them is poorly organized. So since this module's very name implies a poor form of cohesion, my goal will be to completely eliminate it.

In looking at the file (all 40k of it!) I found three major categories of values. The first are things which should really be runtime options -- characterized by DEBUGLOG_LEVEL, in the above sample. These sneak in when we're deep in coding mode and don't want to switch gears, often with a # TODO: move to configuration file comment. With compiled languages, they generally actually to get moved to configuration (or we get busted trying to explain how to enable debug logging in production). With Perl, though, this seems to be just tolerable enough to end up in released software. This category is best dealt with by moving out to configuration in some form. In this particular case, it's very very easy, because the value is actually only used from one place:

Log.pm
... use constant DEBUGLOG_LEVEL => Config->get("debuglog_level"); ...

Next, I found component-specific constants like CC_STATUS_*. These ended up in Constants.pm because some well intentioned person was following the "When in Rome" rule: "looks like all the project constants are in one place; I'd better do the same." Consistency may be desirable, but in this case it is seriously misguided. For absolutely zero gain, this adds an inter-module dependency, namespace pollution, and a global constant. Again, the fix is trivial, because the values of these constants are only used from one module:

CreditCard.pm
... use constant CC_STATUS_SUCCESS => 0; use constant CC_STATUS_CCPROBLEM => -1; ... if ($vrc == CC_STATUS_SUCCESS) { $total_charged += $amount; } elsif ($vrc == CC_STATUS_CCPROBLEM) { $total_failed += $amount; } ...

CreditCard.pm

...
use constant CC_STATUS_SUCCESS => 0;
use constant CC_STATUS_CCPROBLEM => -1;
...
   if ($vrc == CC_STATUS_SUCCESS) {
       $total_charged += $amount;
   } elsif ($vrc == CC_STATUS_CCPROBLEM) {
       $total_failed += $amount;
   }
...

While on the subject: for an excellent (if perhaps somewhat long-winded) argument on the value of keeping scope as small as possible, I recommend section 10.4 of Steve McConnell's "Code Complete 2". It boils down to intellectual manageability: it is easier to understand code when all of the pieces are close at hand.

Also note that this is an improvement regardless of how well- or poorly-organized the destination module might be. Even if CreditCard.pm is total garbage, both it and Constants.pm are simplified and improved by moving the constants closer to where they're used.

Finally, I found constants which are part of some API -- here the RET_* constants are my example, which are used all over the code to standardize on return values from service-layer functions. Their scope truly is global, in this project. Here is where we need to improve the cohesion. Referring back to that wikipedia article, I would suggest that for the RET_* constants, we can achieve communicational cohesion by considering the set of constants as a "datatype" (in quotes because this is Perl, after all...). In other words, we will group together these constants with all functions which operate on them: currently in the KitchenSink::Error module.

While I'm moving to the new module, I am not going to carry over the symbol exporting code. Without getting into the memory bloat problem (which can be really serious), symbol importing and exporting is the wrong kind of laziness: the kind that makes code easier to write at the expense of maintainability, as it obscures (to the human reader of code) the origins of symbols. And since users of these constants will have to namespace qualify them, I did away with the RET_ prefix. So here's my addition to the KitchenSink::Error module:

KitchenSink/Error.pm
... ## "Enumeration" of all of the possible return values ## from the service layer. use constant { SUCCESS => 0, API_ERROR => 1000, INTERNAL_ERROR => 2000, }; ...

Consumers of these values are now:

hundreds of files
... $retval = someServiceMethod(); die KitchenSink::Error::describeRet($retval) unless $retval == KitchenSink::Error::SUCCESS; ...

It'd be possible, too, to make KitchenSink::Error into a "class" (in quotes because this is Perl, after all...), where the consumers could become:

hundreds of files
... $retval = someServiceMethod(); die $retval->describe() unless $retval->isSuccess(); ...

Some might consider this a major improvement; I personally do not, at least in Perl (because it's so weakly typed: the maintainer cannot find out what to expect to find in $retval without looking at the *implementation* of someServiceMethod()). This is, however, basically a matter of taste; one could certainly argue for the class-based approach.

There are also instances (not shown) in the Constants.pm module of constants similar to the RET_* constants, in that they are intended basically as enumeration data types, but where there's no obvious existing module with methods that work on the data type. In these cases, a new module gets created, which just defines (but doesn't export) the enumeration. This is a degenerate case of communicational cohesion: all the functions which operate strictly on the datatype are in one place -- there just aren't any such functions.

If you're balking at the creation of such tiny modules, think of it from the perspective of the maintainer of the software. When something about this datatype breaks, would they rather see it in a module by itself, or mixed in with a bunch of unrelated stuff? Which would make it easier to focus on and comprehend the problem?

In the end, I wasn't able to justify the continued existence of the KitchenSink::Constants module -- mission accomplished. My eyeballs can remain un-gouged-out for a little while longer.

tactics, tactics, tactics

2007-06-17T19:12:00.000-07:00

One of the challenges for professional programmers trying to advance professionally is simply deciding what to study. We each have a limited amount of time to invest in improving our skills. Should we spend those hours reading about architecture? Computing theory? Lambda calculus? Discrete mathematics? Complexity theory? Compilers? Should we get involved in local user groups, or toy around with new programming languages? Join the IEEE Computer Society? It's easy to find folks who will advocate any of those, and they're probably all correct, in the sense that any of those things help. But we can't study everything, so what is the best place to focus our limited time?

Well, I think I have an answer. But first, a detour into chess.

I came to chess as an adult. I'd played it most of my life, but had never studied or played seriously. Just pushed the pieces around, against other players who were also just pushing the pieces around. One day in my late twenties, I decided I would learn to play chess for real. So I went to the library and tried to pick out a book. Even though the SF Main Library had a huge number of chess books, it was surprisingly easy to pick a few: I just started with the ones with broad titles ("chess" and "dummies" come to mind). And I got better, really fast. Suddenly, I could beat the pants off anyone who was just pushing the pieces around.

I was the chess-playing equivalent of a junior programmer, let's say a recently graduated CS student. Unlike people who've never programmed, the junior programmer can usually convince the computer to do what he or she wants it to do. A huge accomplishment! And then the junior programmer does something foolish, like say, gets a job programming computers.

Back to chess, the foolish thing I did was join a chess club, where I was by far the worst player. Now I had the dilemma: what to study next? Like most people in my situation, I chose absolutely wrong. I picked up a book on the Ruy Lopez opening system. After reading it, I was a substantially better Ruy Lopez player. Which was great when I could persuade my opponents to stick to the opening lines I'd studied, which I generously estimate accounted for 0.1% of the games I played. Next I picked another opening, the Grob, and read up on it, and tried to get my opponents to let me play a Grob or a Ruy Lopez. Now 0.2% of my games were improved. And then -- since I'm not crazy in the sense of doing the same thing again and again expecting different results -- I stopped studying specific openings.

By analogy, the employed junior programmer reads a book on yacc and a tutorial on the GNU multiprecision math library. Two things happen: first, that tiny percentage of their time spent working with these specific tools improves, and second, they start to put a lot of effort into convincing co-workers to stick with yacc and libgmp for all tasks. Have they really improved? Yes, but not in a way that is apparent in most of their endeavors.

Fast-forward over the part where I read a big stack of chess books: endgame theory, entire books on specific units, books about the history of chess, books about the psychology of the game, the official USCF rules. Each one improved my games only marginally, if at all, and most ended up as bathroom literature in our house, which really irritates my wife. My rating had barely improved in the few years since I read "Chess for Dummies", and I was still the worst player in the club. I knew that I was looking for a way to improve my game in general, but re-reading the general books wasn't a help, and the advice from people who've mastered chess is really inconsistent. (Frankly, people who write about chess are mostly aging child prodigies, who don't really believe it is possible to learn the game as an adult). Then, by some random stroke of luck, I stumbled on a very thin, very simple book in the book store: "Rapid Chess Improvement", by Michael de la Maza. It's a small book, but I can distill it even further: study tactics. To be fair, the book actually outlines a specific study program, which is great, but the important lesson is just those two words: study tactics.

I don't think this book has been written for computer programmers yet. But let me quickly translate: study coding.

Tactics, in chess, are present in every single game, just as coding is present in all software projects. Tactics are what makes or breaks most games of chess, just as coding is what makes or breaks most software projects. Gambling on a chess game? Put your money on the tactical whiz who has never studied openings over the master of the English opening -- even in an English game! In the same way: hire people who can read, write and talk about code over those with sloppy code and really deep knowledge of J2EE.

Before I move on, I'd like to proudly announce that, after spending 100% of my chess-studying time on tactics for the last year -- maybe 20 hours total -- I'm no longer the worst player in my chess club. I still lose more games than I win, but as long as I've slept and eaten recently, I don't lose because of head-slapping tactical errors. Better still, I am recognized in the club as someone who will punish my opponents' tactical mistakes brutally. Can you guess what I'm studying these days? More tactics.

OK, if you're still with me, you're probably wondering "how does one study coding?"

Chess players study tactics by means of puzzles. A typical tactical puzzle has a picture of a chessboard with a few pieces, and a single sentence problem description: "white to play and win," or something similar. Tactical puzzles are not generally taken directly from actual games. Instead, they are simplified or idealized from real games -- pieces which aren't relevant to the problem are removed, for example, and the real game elements of time pressure and personality and so on are completely removed.

Can this study method be translated into programming? Yes. I believe the equivalent is reviewing code on paper. Not necessarily literally on paper, but in isolation from the compiler, the schedule, the politics and everything else that comes with a professional software project. Just the code. Unlike chess, the problem description is always the same: "how can this code be improved." Ignore the big picture, the product, and high-level software objectives such as performance, portability or reusability. Just think literally: how can the *CODE* be improved, from the perspective of a human reader.

And just like the chess player solving a tactical puzzle, arm yourself! In chess, there are names for all of the common tactical patterns. Fork, pin, skewer. Discovered check, driving off, piling on. The list goes on a bit, but it's finite. In the same way, there is a jargon for describing code quality in much more concrete terms than the all-too-common (and uselessly vague) "bad smell". There are the basics: use meaningful names, keep it simple stupid (KISS), don't repeat yourself (DRY), be consistent, and so on. Then there are more complex quality issues like cohesion, coupling, information hiding, referential integrity and separation of concerns. Challenge yourself to use the correct terminology, both in describing problems and in suggesting improvements.

Note that I'm talking about something you do with the spare time that you've set aside for professional development, not at work. And I certainly don't mean to imply that we should all be applying this kind of scrutiny to every piece of code we produce professionally. At my chess club, we start each game with 25 minutes on the clock. I'd never get past move 10 before losing on time if I applied the kind of focus to each move that I apply when solving a tactical problem, in the same way that software would never be completed if programmers always tried to make it "perfect". The objective of the study, in both cases, is learning to recognize patterns, not learning to apply a specific method.

So daily, weekly, or monthly, pick a manageable chunk of code -- good or bad, yours or someone else's -- print it out (again, not necessarily literally), take it home, and study it in detail, with the objective of improving it. Studying code in this way will not make you way better, immediately, at your current job. Instead, it will make you a little bit better, slowly, at every programming task you undertake.