Moving a problem from one part of your codebase to another does not eliminate it

Posted at 07:00 on 16 October 2014

This is, of course, a statement of the obvious, but I've come across quite a few "best practices" in recent years that violate it.

People come up with some design pattern or other, telling you that it solves some problem or other. At first sight, it appears that it does eliminate the problem from one part of your codebase, but on closer inspection it turns out that it merely shifts it to another, and sometimes even introduces other problems in the process.

I first noticed this in a Web Forms application, where our resident Best Practices Guy berated me for using inline data binding expressions in the .aspx files. These were actually simple data binding expressions, with no business logic, a bit like this:

<asp:Repeater id="rptData" runat="server">
  <p>
    <asp:Label Text="<%# Eval("Text") %>" runat="server" />
  </p>
</asp:repeater>

Just like you've seen in every Web Forms tutorial since 2001, but he said I should have been looking up the label in the DataBound event and assigning it there instead:

void rptData_DataBound(object sender, RepeaterItemEventArgs e)
{
    var label = e.Item.FindControl("lblParagraph") as Label;
    if (label != null)
    {
        label.Text = ((LineItem)e.Item.DataItem).Text;
    }
}

He claimed that it would prevent problems if I'd mistyped the property name in the .aspx file, because the C# compiler would catch it.

The reason this is a fallacy is that it just moves the problem into your code-behind file. You're just as likely to mistype the name of the control -- lblParagraph -- in the string and end up with exactly the same problem. Only it'll be easier to miss it in testing because the null check means that it will fail silently. On top of that, you're using more than twice as many lines of code spread over two different files rather than just one to do the same thing.

I noticed a similar problem when I was evaluating OOCSS -- a design pattern that's supposed to reduce duplication in your CSS, by having you declare separate CSS classes for different functional aspects such as "button" or "highlighted" or "media". Twitter Bootstrap uses it fairly heavily. Its selling point is that it's supposed to make your CSS more maintainable and lightweight without using a pre-processor by reducing duplication in your stylesheets. Unfortunately, in the process, it introduces a lot of duplication and weight into your HTML because you now have to set additional class declarations on a huge number of elements.

Then of course there's our old friend, the Repository Facade, whose proponents tell you that it reduces tight coupling between your business layer and your ORM. Of course a generic Repository Facade does this at the expense of making it impossible to optimise your queries for performance, but with a specialised one -- where you're moving your queries into your Repository Facade itself -- you're just moving the tight coupling from one part of your codebase to another. It doesn't reduce the amount of work that you would have to do to switch your data source in the slightest, and in the process it prevents you from unit testing your business logic independently of the database.