When you write OO code for a living you can find yourself always slipping into that style, even if the problem doesn't warrant it. Recently I turned out some code that fell neatly into this category. I wanted to take some time to compare it with a procedural approach that does the job just as well, but in less code. I'll argue as to why the procedural code is better in this case.
The code I wrote (C#) performed some complex logic to select a subset of things from a database for a given client. The rules for selection differed between clients. Stripping away all the logic, below is the shape of the code I came up with.
// Abstraction of the underlying database.
public interface IDatabase { }
// Thing we want to get from the database.
public class Workspace { }
// Interface defining a "selector" that gets
// things from the database.
public interface ISelectWorkspaces
{
List<Workspace> Select();
}
// Concrete selector containing logic to
// fetch client X workspaces.
public class SelectWorkspacesForClientX : ISelectWorkspaces
{
public SelectWorkspacesForClientX(IDatabase db) { }
public List<Workspace> Select()
{
// ...
}
}
// Concrete selector containing logic to
// fetch client Y workspaces.
public class SelectWorkspacesForClientY : ISelectWorkspaces
{
public SelectWorkspacesForClientY(IDatabase db) { }
public List<Workspace> Select()
{
// ...
}
}
// Interface for a factory class that makes
// selectors.
public interface ICreateSelectors
{
ISelectWorkspaces Create(string selectorName);
}
// Concrete factory class that makes selectors
// given a client name.
public class SelectorFactory : ICreateSelectors
{
private IDatabase _db;
public SelectorFactory(IDatabase db) { _db = db; }
public ISelectWorkspaces Create(string selectorName)
{
ISelectWorkspaces selector;
switch (selectorName)
{
case "client_x":
selector = new SelectWorkspacesForClientX(_db);
break;
case "client_y":
selector = new SelectWorkspacesForClientY(_db);
break;
default:
throw new ArgumentOutOfRangeException("SelectorName");
}
return selector;
}
}
// Entry point to this part of the
// application. We need to pass in
// the selector factory instance.
public void DoStuff(ICreateSelectors selectorFactory)
{
// ...
var selectorName = ConfigurationManager.AppSettings["SelectorName"];
var selector = selectorFactory.Create(selectorName);
var workspaces = selector.Select();
// ...
}
What can we say about this code? Well, it's definitely OO. There are seven separate entities here, each being defined in their own file. Then there is the logic that uses these entities to actually do something in its own file. So to comprehend this, we need to read the code of eight files. Sure each one is short, but that's a lot of jumping around in a codebase.
We have interfaces (so we can mock in tests and do dependency injection), and a factory because, well, you always need a factory right? In this case it is really nothing more than a glorified switch statement.
There is nothing really wrong with this code. It is clearly laid out and logical. It's just mostly unnecessary. An interface that has only one method on it can be expressed as a function signature. A class that has a constructor and a single method is a glorified function definition.
How would a straight forward procedural version of this code look?
Here's the procedural version.
// Abstraction of the underlying database.
public interface IDatabase { }
// Thing we want to get from the database.
public class Workspace { }
// Function containing logic to select client X workspaces.
public List<Workspace> SelectWorkspacesForClientX(IDatabase db)
{
// ...
}
// Function containing logic to select client Y workspaces.
public List<Workspace> SelectWorkspacesForClientY(IDatabase db)
{
// ...
}
// Entry point to this part of the
// application. We need to pass in
// the database instance.
public void DoStuff(IDatabase db)
{
// ...
var selectorName = ConfigurationManager.AppSettings["SelectorName"];
Func<Database, List<Workspace>> selector;
switch (selectorName)
{
case "client_x":
selector = SelectWorkspacesForClientX;
break;
case "client_y":
selector = SelectWorkspacesForClientY;
break;
default:
throw new ArgumentOutOfRangeException("SelectorName");
}
var workspaces = selector(db);
// ...
}
Two entities and three functions. With the exception of the database abstraction and workspace class, everything lives in one file. It turns out this was the only place in the codebase these selectors where used. To that end I inlined the factory as we didn't need to reuse it. Even if code reuse were an issue, we could move the switch out to its own function and move it to a file with the selector functions. Tada, code reuse without the faff.
What I like about this version is there are less conceptual elements in play. No factory interface and class, no interface and classes for the selectors. Just basic functions and a control flow statement. Any developer (familiar with the language or not) should be able to follow this.
A lot of OO abstraction is speculative. We add interfaces and inheritance on the off chance we will want to change things in the future. Adding that stuff later on is no hardship. Abstractions should arise from the code that is written, not be written in advance just in case. Focus more on doing the thing that needs to be done. When it gets too complex, or too messy, that's when the abstractions come in to play. Not before.