The interface not supported fallacy
Today I was reviewing a colleague’s code and I saw a pattern similar to the one below:
interface IWorker
{
void doWork();
void setOfflineTarget(string target);
}
class OnlineWorker : IWorker
{
void doWork()
{
// do some work online
}
void setOfflineTarget(string target)
{
throw new NotImplementedException();
}
}
class OfflineWorker : IWorker
{
string target;
void doWork()
{
// do some work offline using this.target
}
void setOfflineTarget(string target)
{
this.target = target;
}
}
Did you notice the code smell? I was hoping to find a quick blog post online and send it my colleague’s way - certainly if some stranger wrote it on the web, it will be more convincing than I could ever be. Maybe I am getting rusty on my search keyword selection, but I didn’t find anything explicit so I decided to write this one.
Back to the problem, we don’t need to see how these workers classes are consumed to start noticing the problem. setOfflineTarget method on the interface is clearly out of place as it doesn’t fit the abstraction the interface indents - that is just do work.
The interface’s goal is to expose a common, well-defied abstraction by projecting common capabilities between all the instances that implements it. If a significant portion of the implementations cannot provide a functionality that the interface specifies, then it is an indication that that functionality doesn’t belong to such abstraction.
This also means that the caller must have broken the abstraction in some fashion. Maybe the caller looks like this:
var worker = GetWorker();
if (worker is OfflineWorker)
{
worker.setOfflineTarget("some value");
}
or this
var worker = GetWorker();
try
{
worker.setOfflineTarget("some value");
}
catch (NotSupportedException)
{
// who could ever know?
}
In which case, there is little point of having such an interface.
It seems to me that, ideally, no interface method would go unimplemented - but I guess in some cases the convenience might be worth the trouble. Care must be taken, though, breaking abstractions over time will just speed up design erosion - that is - the not so yummi spaghetti code.
There are likely many ways to solve this. But I guess the best recommendation I could give is check your abstractions.