Lately I’ve been wrapping up interviews with the question “What makes software maintainable?” I usually catch a glimpse that says “hmm… I’ve never really thought about maintainability” followed by a response to the question “What makes software good?”, which I didn’t actually ask, but most programmers have thought enough about to give an on the spot answer. The bland responses inevitably involve documentation (fail) and maybe something about consistency (meh).

The underlying question I’m asking is a question you should ask yourself every time you write a line of line of code. That question is “What are you optimizing?” Most programmers have mentally hard-coded the term optimize to mean efficient use of memory and cpu, however it’s a horribly myopic definition. In fact, it’s just plain wrong unless you’re writing an iPhone game or maybe you work at Google and saving a few cpu cycles translates to kilowatts of power and possibly saving a small rain forest.

Let’s see what Wikipedia has to say.

In computer science, program optimization or software optimization is the process of modifying a software system to make some aspect of it work more efficiently or use fewer resources.[1] In general, a computer program may be optimized so that it executes more rapidly, or is capable of operating with less memory storage or other resources, or draw less power. — http://en.wikipedia.org/wiki/Optimization_(computer_science)

It’s not wrong, but it reinforces the notion that less is more and the precious resources are physical resources like memory and cpu. Is it better to have fewer files in your project, fewer dlls in your bin directory, or use fewer semicolons? I recently debugged a chain of jQuery statements that was 430 characters long (un-minified) and used one semicolon. Is that optimized?

Consider this definition:

In mathematics, computer science and economics, optimization, or mathematical programming, refers to choosing the best element from some set of available alternatives. — http://en.wikipedia.org/wiki/Optimization_(mathematics)

This does a better job of capturing the trade-offs we make on a daily basis. I like the idea of choosing the best element rather than consuming the fewest resources. Instead of asking what you’re optimizing, a better question might be what are you maximizing? For a new iPhone game, maybe time to market is your number one concern. I work on enterprise SaaS applications that have a long shelf life so maintainability is a pretty big deal. I strive to maximize readability and testability when I write software. What are you maximizing?

Take a look at this function:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17

public UserIndexViewModel GetIndexViewModel(int pageSize, int pageNumber) {
    var model = new UserIndexViewModel();

    var allUsers = Session.CreateCriteria<User>();

    model.Total = CriteriaTransformer
        .TransformToRowCount(allUsers)
        .UniqueResult<int>();

    model.Users = allUsers
        .SetFirstResult(pageSize * (pageNumber - 1) + 1)
        .SetMaxResults(pageSize)
        .AddOrder<User>(u => u.LastName, Order.Asc)
        .List<User>();

    return model;
}

There isn’t much going on here, but it could be better. I’m looking at the arguments pageSize and pageNumber. Notice how they both start with the prefix “page”. That’s a smell. The common prefix means the arguments are related, so let’s apply the Introduce Parameter Object refactoring and see what happens.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21

public class Page {
    private readonly int number;
    private readonly int size;

    public Page(int number, int size) {
        this.number = number;
        this.size = size;
    }

    public int Size {
        get { return size; }
    }

    public int Number {
        get { return number; }
    }

    public int FirstResult {
        get { return size * (number - 1) + 1; }
    }
}

What did we accomplish?

• Creating the parameter object gave us a place to put the logic of calculating the first result of the current page. This logic would otherwise be scattered around the application and even though it’s simple, it’s error prone.
• The logic of calculating the first result is testable.

1
2
3
4
5
6

[Test]
public void Should_calculate_first_result_of_current_page() {
    Assert.That(new Page(1, 20).FirstResult, Is.EqualTo(1));
    Assert.That(new Page(2, 20).FirstResult, Is.EqualTo(21));
    Assert.That(new Page(3, 20).FirstResult, Is.EqualTo(41));
}

It’s not rocket science, but it still deserves a test. Our other option is spinning up Cassini and clicking around to make sure we got it right. That’s just silly. A little piece of me dies each time I press F5 to run a manual test.

Anyway, there is this thing that happens with single responsibility classes. Because their purpose is crisp, it draws out robustness. In this case, I’m thinking about what we should we do if we get zero or a negative value for page number. I’m a fail fast kind of guy, so I’d probably go with a guard clause.

1
2
3
4

public Page(int number, int size) {
    GuardAgainstLessThanOne(number);
    ...
}

But you could silently correct it if that’s how you roll:

1
2
3

public int Number {
    get { return Math.Max(1, number); }
}

The point is that this is an obvious scenario to consider when looking at such a special purpose object. All too often though, bits of logic are mixed together like 32 kindergarteners sitting next to each other on a rug during a lesson. The room is one fart or tickle away from erupting into total kaos. It’s roughly half teaching and half crowd control, only without the rubber bullets and pepper spray. Not surprisingly, overcrowded classrooms don’t work that well and over-ambitous functions don’t work that well either. Each little bit of logic needs some one on one with you. Give it a chance to be all it can be by pulling it aside and getting to know it a little better. You might be surprised.

The title of this article was inspired by The Absolute Minimum Every Software Developer Absolutely, Positively Must Know About Unicode and Character Sets (No Excuses!) by Joel Spolsky

I recently saw this question on stackoverflow:

There were good points on both sides, but the majority of responses were from closet interface haters succumbing to the mob effect to say “me too”. On the other side of the argument, many took cover behind regurgitated responses like interfaces are for mocking dependencies in unit tests and because people that write books say you should use them. So how is it that scores of professional software developers don’t understand or can’t communicate why and how to use the most fundamental of object-oriented language features? Perhaps there is a clue in the details of the original question:

Is there some hidden benefit of using an interface when you have 1 version of a class and no immediate need to create an interface?

The scattered and ineffectual responses are an unfortunate casualty of the ubiquitous poor online examples found on the Internet. No doubt you’ve seen something like this before:

1
2
3
4
5
6
7
8
9
10
11

public interface IVehicle {
    int NumberOfPassengers { get; }
}

public class Car : IVehicle {
    public int NumberOfPassengers { get { return 4; } }
}

public class Bus : IVehicle {
    public int NumberOfPassengers { get { return 20; } }
}

I can’t help but have visions of rich objects that model the physical world so well they put an end to wars and world hunger. You know, methods like Bark() on a Dog object. These examples aren’t wrong per se, but they usher the reader down a predictable and dead-end path. It’s like the horribly cliché nine dots puzzle in which the goal is to link all 9 dots using four straight lines or less, without lifting the pen.

Common sense suggests it can’t be done because we have subconsciously imposed a false restriction that the lines must be within the boundaries of the square (I won’t insult you with the hopelessly worn out catch phrase). The point I’m trying to make here is the examples out there unwittingly set us up for failure. In social science this is called framing. The human brain is an impressive problem solving machine, but we can be easily fooled by framing a problem in a way that our stereotypes and assumptions obscure the often obvious answer.

In the case of interfaces, the examples suggest the concrete implementations are mutually exclusive physical concepts like Car and Bus, or perhaps SqlUserStore and LdapUserStore. It’s an easy concept to latch on to, and it’s the bane of good object oriented design.

To think of a user store as a thing is a mistake. IUserStore defines a seam in which responsibility crosses a boundary. In an MVC application, it’s the seam between the controller’s routing responsibility and the model’s business logic. If you only have one implementation of IUserStore, then you’d better take another look because that’s a smell.

The place to start looking is within your one and only implementation. Where is the logic about sending a confirmation email or checking if the username is available? Chances are it ended up in the controller leaving your model anemic or it ended up in your model violating the single responsibility principle. Either way it’s a big fail sandwich.

Again, this is because of the false assumption that implementations are mutually exclusive as in the Car and Bus example. In other words, if I have a Car, then I don’t have a Bus. This is the wrong way to think of objects. Of course they can be mutually exclusive, but they are far more powerful when they are composable. Consider these implementations of IUserService:

• EmailConfirmationUserService
• ValidatingUserService
• SqlUserService
• CachingUserService

Each concern is orthogonal to each other concern and they are combined together to create the overall behavior of the system. The interface facilitates composition which is the key to high-quality, robust software. So the real problem isn’t that interfaces are overused, it’s that interfaces are squandered.

The DataAnnotationsModelBinder is a pretty neat little class that enforces validation attributes on models like this one:

1
2
3
4
5
6
7
8
9
10

public class Product {
    public int Id { get; set; }

    [Required]
    public string Description { get; set; }

    [Required]
    [RegularExpression(@"^\$?\d+(\.(\d{2}))?$")]
    public decimal UnitPrice { get; set; }
}

Just for the record, this is a cool bit of code that was released to the community for backseat drivers like me to use and complain about. I’m really only bitter because I can’t use it. Let’s take a peek at the source code to see why:

1
2
3
4
5
6
7
8

/// <summary>
/// An implementation of IModelBinder which is designed to be a replacement
/// default model binder, adding the functionality of validation via the validation
/// attributes in System.ComponentModel.DataAnnotations from .NET 4.0.
/// </summary>
public class DataAnnotationsModelBinder : DefaultModelBinder {
    ...
}

The problem with this class is that it’s not just about validation. It’s more about binding with validation sprinkled in. The author even states this as a design goal with the comment “designed to be a replacement default model binder.” This is a violation of the Single Responsibility Principle and it’s the reason I can’t use it. The DefaultModelBinder does some basic reflection mapping of form elements to model properties. That’s cool, but what about a controller action that takes json, xml, or anything slightly more complicated than what the default binder can handle?

I’ll tell you what happens. You write a custom model binder which is pretty easy and happens to be a pretty rad extension point in the MVC framework. Consider this model binder that deserializes json:

1
2
3
4
5
6
7
8
9
10
11
12

public class JsonModelBinder<T> : IModelBinder {
    private string key;

    public JsonModelBinder(string requestKey) {
        this.key = requestKey;
    }

    public object BindModel(ControllerContext controllerContext, ...) {
        var json = controllerContext.HttpContext.Request[key];
        return new JsonSerializer().Deserialize<T>(json);
    }
}

That’s pretty cool, but now I’ve lost my validation! This my friend is the evil of inheritance and the epic fail of the DataAnnotationsModelBinder. So what’s wrong with inheritance, isn’t that what OOP is all about? The short answer is no (the long answer is also no.)

Human sacrifice, dogs and cats living together… mass hysteria! — Dr. Peter Venkman

Inheritance is pretty enticing especially coming from procedural-land and it often looks deceptively elegant. I mean all I need to do is add this one bit of functionality to another other class, right? Well, one of the problems is that inheritance is probably the worst form of coupling you can have. Your base class breaks encapsulation by exposing implementation details to subclasses in the form of protected members. This makes your system rigid and fragile.

The more tragic flaw however is the new subclass brings with it all the baggage and opinion of the inheritance chain. Why does a model that enforces validation attributes care how the model is constructed? The answer is it shouldn’t, and to add insult to injury, the better design is actually easier to implement. Listen closely and you may hear the wise whispers of our forefathers…

Use the decorator pattern.

Is this really going to solve all my problems? The short answer is yes. Let’s take a look:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15

public class BetterDataAnnotationsModelBinder : IModelBinder {
    private IModelBinder binder;

    public BetterDataAnnotationsModelBinder(IModelBinder binder) {
        this.binder = binder;
    }

    public object BindModel(ControllerContext controllerContext, ...) {
        var model = binder.BindModel(controllerContext, bindingContext);
        AddValidationErrorsToModelState(model, controllerContext);
        return model;
    }

    ...
}

Now all this class does is enforce validation and we can use whatever model binder we want to construct our object. Let’s wire this up in the Global.asax real quick:

1
2
3
4
5

var productModelBinder = new JsonModelBinder<Product>("product");

ModelBinders.Binders.Add(
    typeof(Product),
    new BetterDataAnnotationsModelBinder(productModelBinder));

This is cool for the following reasons:

1. We are not coupled to the DefaultModelBinder. This means we don’t know or care about the structure and implementation details of it so it’s free to change in the next version of ASP.NET MVC.
2. The code is simpler because it is only about validation.
3. The behavior is composable. This means we can add validation to any model binder (like our json model binder) and bolt on more responsibilities with a chain of decorators. How about converting DateTime fields to UTC or striping formatting characters from phone numbers?
4. It’s easy to test.
5. You can substitute a decorator for a subclass in most cases and in most cases it’s the right choice. You’re better off making the decorator your default choice and only settle for inheritance when you really need it.

…don’t be jealous that I’ve been chatting online with babes all day. Besides, we both know that I’m training to be a cage fighter. — Kip

The default model binder or DataAnnotationsModelBinder in ASP.NET MVC works well for simple data structures that basically map properties directly to form elements, but it quickly breaks down in a real world application. Take this simple scenario:

The built-in model binder can handle the easy fields like FirstName, LastName, and Email, but it doesn’t know how to deal with the Roles. This is where the custom model binder comes in:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27

public class UserModelBinder : IModelBinder {
    private readonly IModelBinder binder;

    public UserModelBinder(IModelBinder binder) {
        this.binder = binder;
    }

    public object BindModel(ControllerContext controllerContext, ...) {
        var user = (User)binder.BindModel(controllerContext, bindingContext);
        AddRoles(user, controllerContext);
        return user;
    }

    private static void AddRoles(User user, ControllerContext controllerContext) {
        foreach (var role in GetRoles(controllerContext)) {
            user.Add(role);
        }
    }

    private static IEnumerable<Role> GetRoles(ControllerContext controllerContext) {
        var roles = controllerContext.HttpContext.Request["roles"];
        if (roles == null) yield break;
        foreach (var roleId in roles.Split(',')) {
            yield return (Role)Convert.ToInt32(roleId);
        }
    }
}

And it gets wired-up in the Global.asax like this:

1
2
3

var defaultBinder = new DataAnnotationsModelBinder();
ModelBinders.Binders.DefaultBinder = defaultBinder;
ModelBinders.Binders.Add(typeof(User), new UserModelBinder(defaultBinder));

Because UserModelBinder is a decorator, it preserves the base functionality (like validation) of the default binder and only adds the part about mapping the roles. This is good stuff because it keeps construction of the model out of the controller. That’s the Single-Responsibility Principle in action.

Command Query Separation

The two functions AddRoles, and GetRoles are an example of command query separation (CQS). It would be easy to combine those two functions together and in fact that’s how it looked at first. So why bother separating them? Well it doesn’t really matter in this simple case, but it’s like how I tell my 3 year-old son to move his milk cup away from the edge of the table. I figure it’s a good habit that will prevent a few spills in the long run. The idea with CQS is there is a natural seam between finding objects and doing stuff with them. I buy that. I expect GetRoles to be more volatile than AddRoles or at least change for different reasons.