Kevin Rohrbaugh

Occasionally, I like to go back and review my code after it’s had some time to age. This can be a great way to see how you’ve personally evolved as a developer. Typically this process happens during maintenance through ongoing refactoring, and can be hard to notice. Sometimes, however, there are little applications that may not get worked on much and, since they do their job, they just chug along in production for year after year. These are what I think of as code fossils: code that got stuck in tree sap and became a preserved snapshot of your former self.

A code fossil of my own is the caching system I built using .NET 1.1 in the summer of 2004. What follows is a short synopsis and critique about the code.

Requirements

The application I was building was a rather simple content-aggregation web application. One of the business requirements was for it to “be fast” which typically translates to “is cached”. At a high level, here are some requirements:

• Content retrieved from other systems must not access those systems for every request served (i.e. content must be cached locally to minimize load on source systems).
• Should source systems be unavailable at the time of cache expiration, cached content should be retained and used.

From these basic requirements, I seem to have added my own:

• Cache configuration should be flexible, so that it’s possible to disable caching globally or change where any given piece of content is cached using a configuration file.
• Cache settings, such as expiry method and duration, should also be configurable without code changes.

Implementation

From these requirements (including those that I made up myself) I built the ContentManager class and it’s partners in crime, the CacheManger and IDataLoader:

Essentially, these classes (along with a few others) worked together so that the application code would request a piece of content using the GetContent() method on the ContentManager singleton (ContentManager.Manager). The ContentManager then determined if said content was cached and invoked the LoadContent() method on CacheManager’s singleton, which either retrieved the content from cache or triggered the ContentNotFound event. The ContentNotFoundEvent then, through a strange design decision, invoked the ContentManager to load the content using an IDataLoader that was resolved using some XML configuration in custom sections of the Web.config file for the application.

All of this resolution was done with three layers of configuration: content mappings, cache categories and content loaders. Content mappings linked pieces of content to a cache category, which specified where and how content was cached (for instance ASP.NET Application, Session or Cache objects). Content loaders mapped content to classes that loaded the content when it wasn’t available in the cache. As outlined above, all of this was done using custom configuration sections in the Web.config, meaning that it involved several more classes to map all this information within the application.

Critique

This design makes me squirm, to the point that I was hesitant about posting it at all. First of all, when it’s all said and done there are ten classes dedicated to all this mapping, configuration and dynamic caching. To cap it all off, the method signature for GetContent (and related) looks like this:

public object GetContent(string contentKey)
{
    object content = null;

    // Redacted to protect the innocent

    return content;
}

That’s right, the entire system hinges on (case-sensitive) magic strings and returns object.

In the end, here are some things that immediately pop to mind when I look at this code today:

• YAGNI & KISS: The truth is, a lot of this caching code was just fun for me to play with. I was brand new to ASP.NET at the time, so I got to learn about custom configuration sections and all the various options you can set when putting items in System.Web.Caching.Cache. In the end, though, these settings never got tweaked once the application development was done and there were better things I could have spent my time on. It’s true that being able to disable caching was useful during the development cycle, but I wasted a lot of time building an overly flexible cache system that simply wasn’t needed.
• Avoid anti-patterns and use design patterns sensibly: Heavy reliance on the magic string anti-pattern for all the cache mapping resulted in a brittle system that tightly coupled URL patterns to the underlying content. Additionally, while singletons are useful in some situations, applying them here makes it difficult to unit test any code that uses these classes (which, in this case, is the rest of the application). Not surprisingly, this application has very few unit tests.
• Use Output Caching: The saddest part of this entire endeavor is that all that’s being cached here is the content to be displayed, in the domain objects used to display it. In other words, the content is still being rendered by ASP.NET on every request. Since performance concerns were the central business need and much of the content was largely static, output caching would have likely proved more beneficial, even with the drawbacks it had in ASP.NET 1.0.
• Class naming: Naming a class is often challenging as it tends to set the tone for what its purpose is. When you name something with the Manager suffix, it’s likely that you don’t know what exactly the class is supposed to be doing, which means it’s likely to end up doing more than it should. Case in point: The ContentManager is what other parts of the application call to retrieve content, and it’s also what is invoked (through an event) when a cache miss occurs.

What I’d Change

This isn’t meant to be a full design review, so I won’t outline a complete design for how I’d implement this today, but instead offer a brief overview.

In place of ContentManager and CacheManager, I’d employ service objects that exposed the actual domain objects for the specific pieces of content. The service objects wouldn’t use static methods and would be passed into the page presenter objects using dependency injection. To add caching, I’d either use AOP or sub-classes of the service objects that perform caching duties, most likely using keys generated from class and method names and not exposed to the user in any form. This would make it possible to control caching using the IoC container or AOP framework, rather than a host of custom XML and configuration classes. In most cases, the service objects would call out to repository objects that would retrieve the content from the appropriate data store (taking the place of IDataLoader). 

I wouldn’t bother with supporting overly flexible cache settings as the need doesn’t exist. I’d also employ output caching much more heavily and only use the above caching strategy for those content areas with external dependencies, as the requirements dictate.

Closing Thoughts

To be honest, I’ve never looked at any code I’ve written months ago and felt anything better than mild contempt toward it. Perhaps I’m just not that talented a developer, but I suspect that this is probably the case for most of us.

At the end of it all, the dirty little secret is the caching system is still chugging along in production, doing its (not so elegantly designed) job every day. While code fossils are fun as they highlight how you're development technique has evolved, it’s also important to remember that they aren’t necessarily past failures. After all, dinosaurs may not have proven to be the most resilient life-forms around, but they can inform us about today’s world and did pretty well for 160 million years or so.