Why You Can Throw Away "If", and Why You Shouldn't

Introduction

Most reasonably experienced object-oriented programmers have probably stumbled upon the same realization; namely, that it's possible to replace if statements with polymorphism. Polymorphism is simply a way to delay a decision until runtime. The if statement does the same thing. In fact, procedural programmers need to resort to things like if and switch statements because they have no other tool. Functional programmers, on the other hand, simply toss functions around willy nilly.

This realization can be powerful. It can also really hurt a code base (I know - I've smashed my share of algorithmic china with this hammer). I recently ran into a place on a project where it was a great idea, and I thought I would share why I thought it worked so well.

Current Implementation

Suppose you have 2 methods:

public void storeNewItem() {
    Item item = new Item();
    item.name = request["name"];
    item.description = request["description"];
    item.quantity = request["quantity"];
    item.value = someComplexCalculation();
    item.totalValue = item.quantity * item.value;
    // calculate and store some more fields here
    items.addNewItem(item);
}

public void storeExistingItem() {
    Item item = items.get(request["itemId"]);
    item.name = request["name"];
    item.description = request["description"];
    item.quantity = request["quantity"];
    item.value = someComplexCalculation();
    item.totalValue = item.quantity * item.value;
    // calculate and store some more fields here
    item.update();
}

These two functions should look pretty similar. In fact, they are nearly identical. Both acquire an item, populate it with data, and then store it. The only difference is the way that the item is acquired and the way that the item is stored.

First Attempt

I wanted to merge these methods, and this was my first attempt.

public void storeItem() {
    Item item;
    if (request["itemId"] == null) {
        item = new Item();
    } else {
        item = items.get(request["itemId"]);
    }

    item.name = request["name"];
    item.description = request["description"];
    item.quantity = request["quantity"];
    item.value = someComplexCalculation();
    item.totalValue = item.quantity * item.value;
    // calculate and store some more fields here

    if (request["itemId"] == null) {
        items.addNewItem(item);
    } else {
        item.update();
    }
}

This works, but is obvious crap. I found myself saying "I wish Item were able to handle those details by itself".

Second Attempt

Well, I wasn't brave enough to change Item, so I instead wrapped it.

public void storeItem() {
    Persister persister;
    if (request["itemId"] == null) {
        persister = new NewItemPersister();
    } else {
        persister = new ExistingItemPersister(request["itemId"]);
    }

    Item item = persister.getItem();
    item.name = request["name"];
    item.description = request["description"];
    item.quantity = request["quantity"];
    item.value = someComplexCalculation();
    item.totalValue = item.quantity * item.value;
    // calculate and store some more fields here
    persister.persist();
}

interface Persister {
    Item getItem();
    void persist();
}

class NewItemPersister implements Persister {
    private Item item = new Item();
    
    public Item getItem() { return item; }
    
    public void persist() { items.addNewItem(item); }
}

class ExistingItemPersister implements Persister {
    private Item item;
    
    public ExistingItemPersister(String itemId) {
        item = items.get(request["itemId"]);
    }
    
    public Item getItem() { return item; }
    
    public void persist() { item.update(); }
}

We still have an ugly if at the top of the function, and we have certainly ballooned the code. I still think that this is better than what we started with.

• There is less duplication, which will make maintenance here much easier.
• The Persister interface could be made into a generic class, and the implementations could be re-used all over the system. Some reflection here could really simplify your life.
• A good web framework would allow you to remove that pesky initial if statement. In a less good framework, you could hide this behind some sort of object that knows how to generate a persister from an itemId (or null).

The practical upshot is that these changes should make it easier to apply metaprogramming techniques to this chunk of code. The only code that can't really be made declarative is some of the code which assigns values to fields.

There is one thing that bothers me, though. We have made the Persister implementors responsible for the lifetime of the Item. That's not at all clear from the interface, but it is obvious from the use. The tell-tale sign is that we have a getItem() method. Getters that expose their class' internals like this are evil, and if you don't believe me, you're just plain wrong. I won't try to justify that statement in this post, but trust me.

Third Attempt

To solve this, we could change the interface yet again (and I will switch to Javascript, because Java doesn't have any convenient lambda syntax).

function storeItem() {
    if (request["itemId"] == null) {
        var persister = newItemPersister;
    } else {
        var persister = new ExistingItemPersister(request["itemId"]);
    }
    
    persister.update(function(item) {
        item.name = request["name"];
        item.description = request["description"];
        item.quantity = request["quantity"];
        item.value = someComplexCalculation();
        item.totalValue = item.quantity * item.value;
        // calculate and store some more fields here
    });
}

var newItemPersister {
    update:function(f) {
        var item = new Item();
        f(item);
        items.addNewItem(item);
    }
}

function ExistingItemPersister(itemId) {
    this.itemId = itemId;
}

ExistingItemPersister.prototype.update = function(f) {
    var item = items.get(request["itemId"]);
    f(item);
    item.update();
}

Now, the item's lifetime is only as long as a call to update() is on the stack. This is a common idiom in Ruby, as well.

Conclusion

In the end, I wasn't completely happy with any of these solutions. I think that things are better than they were before. There are also a number of other permutations that will get it marginally closer to ideal. I think that the real solution is to update Item so that you can create a new item and save an existing item with a single method. After that, the code to choose whether to create a new object or fetch an existing object should be minimal and extractable.

I did learn a rule of thumb for deciding when to replace an if statement with polymorphism. If you find yourself saying "I don't want to deal with this, I wish it were handled for me," there's a good chance that you could benefit from some polymorphism. Also, if you find yourself checking the same condition multiple times in a function (as we had in the original implementation), you might want to consider whether polymorphism will help you.