Chocolate for Dogs

I wrote last night about a quick way to find Liskov Substitution Violations (LSP). Well, what good is it to find them if you don’t have a clue how to fix it? (Well, surely you do have some idea how to fix them, but I need some kind of segue into this, don’t I?)

The example in the previous post was a highly-anonymized bit of code from work that I came across a while ago. Kept sticking in the back of my mind — namely because my predecessor had put a comment right above the bit of code saying “This violates LSP, fix ASAP.” (Of course, he never got around to fixing it, nor did I ever really consider it myself — everything worked, right?)

Well, this past weekend I was playing around with NRefactory, a C# parser library from the folks who develop SharpDevelop. The whole library is built on the Visitor pattern, a pattern I wasn’t very familiar with in practice until I started trying to parse source code this weekend. I started playing with it, and at some point in time my mind went to that little snippet of code my ex-coworker had commented. I realized that — aside from having too little in my life to distract me from work (it’s the weekend, for Christ’s sake) — the visitor pattern would be a perfect solution for that little snippet of code.

Let’s get back to last night’s example though, which I’m going to put here again, slightly modified to make things a little more obvious:

[sourcecode language="csharp"]
public abstract class BaseObj { }

public class FooObj : BaseObj { }

public class BarObj : BaseObj { }

public class Worker
{
private void SomeMethod(Dictionary inputParams)
{
BaseObj obj = MyFactory.GetObject(inputParams);
if (obj is FooObj)
{
DoFooStuff(obj as FooObj);
}
else // obj must be a BarObj’s
{
DoBarStuff(obj as BarObj);
}
}

private void DoFooStuff(FooObj foo) { /* does stuff with a FooObj */ }

private void DoBarStuff(BarObj bar) { /* does different stuff with a BarObj */ }
}
[/sourcecode]

As an aside, I do want to point out that this is a blatant Open-Closed Principle (OCP) violation as well (and, generally speaking, an LSP violation is also an OCP violation).

If you’re not familiar with the Visitor pattern, it’s really simple: you call a method on one class, passing your current instance (the “this” variable) to that method; that method, in return, calls another method on the “this” object you’d passed in, again passing what is now the current “this” object back.

Clear as mud? Okay, it’s a lot easier to see in code here:

[sourcecode language="csharp"]
public abstract class BaseObj
{
public abstract void VisitWorker(Worker w);
}

public class FooObj : BaseObj
{
public override void VisitWorker(Worker w) { w.DoFooStuff(this); }
}

public class BarObj : BaseObj
{
private FooObj[] fooObjs;
public override void VisitWorker(Worker w) { w.DoBarStuff(this); }
}

public class Worker
{
private void SomeMethod(Dictionary inputParams)
{
BaseObj obj = MyFactory.GetObject(inputParams);
obj.VisitWorker(this);
}

public void DoFooStuff(FooObj foo) { /* does stuff with a FooObj */ }

public void DoBarStuff(BarObj bar) { /* does different stuff with a BarObj */ }
}
[/sourcecode]

Make a bit more sense? It was easy to solve not only the LSP violation, but the OCP violation as well, just by moving the calls to “DoFooStuff” and “DoBarStuff” into the FooObj and BarObj classes. At worst here, we have an Dependency Inversion Principle (DIP) violation because we have two methods that are relying on concrete implementations (FooObj and BarObj) instead of their common abstraction (BaseObj). This probably isn’t much of a problem here, since we’re apparently needing different functionality or information from FooObj that BarObj isn’t providing (or vice versa).

The only overtly “bad” thing about this implementation is that it requires our previously private members to now be public — or, at least, internal — so that the other classes can access them. I know there’s a lot of developers that think that the keyword “private” is holy, and reserve “public” access only for those godforsaken times you can’t avoid it — I’m all for encapsulation, but I don’t mind exposing functionality when it gives a benefit such as the above.

A couple of other criticisms here though: First, this example is kinda bad, in that we now have a highly coupled system. Your best bet would be to abstract the DoXStuff into an interface, and have the Visitors rely on that interface instead of directly on the Worker implementation.

Second, in spite of what I said earlier, there’s still a slight OCP violation (or, at least, the possibility for one). If you add another branch to the BaseObj tree straight from the root, you’re going to need to either add another method to the Worker class to handle it (i.e. DoNewObjStuff), rely on existing functionality, or create a degenerative method for VisitWorker (degenerative method == overridden method that does absolutely nothing, just returns). If you decide to abstract the DoXStuff to an interface, as suggested above, that’s fine, but you’ll need to add the functionality to your new interface and then change all implementations of that interface to provide the new method. Of course, your new descendant isn’t required to use a DoXStuff method from the worker — it can consume any method available to it.

A little painful, true, but it’s a hell of a lot cleaner than having to your if…else if… branch just because you added a new class. And if the new class needs additional functionality in the Worker class, that’s just par for the course — you’d have to add it one way or another.

Okay, I hope that made some kind of sense, as I’m out of time to elaborate further. Off to the garage for a smoke, then off to the grind again.