Friday, September 23, 2011

An example of inheritance gone wrong

In an earlier post entitled Why inheritance for code reuse should be avoided I pointed out that the key problem was conforming to the Liskov Substitution Principle.

Although I guess there is nothing controversial in what I wrote, I fear I probably did not convince anyone who wasn't already convinced.

Today at work, I had the pleasure of being hit by a bug that is a wonderful example of inheritance gone wrong.

Let me give a bit of context. Once upon a time there was an application that allowed viewing and editing a database in a nice and user-friendly somewhat buggy GUI. As it was felt the application could be user-friendlier and more stable, additional development efforts were planned to improve it. This logically led to the decision of distributing what was a stand-alone file-oriented program over a server-client architecture.

In case you hadn't noticed, I'm being a bit sarcastic, but the fact is that the original plans for the application included extensions to a server-client architecture, and the source code was cleanly organized in modules, keeping the GUI and the database separated by a middle layer.

When moving to the new architecture, it felt natural to turn the middle layer into a web service. This required all data types in the API of the middle layer to be serializable so that they could be sent over the net between the client (the GUI) and the server. This was fairly easily achieved using WCF and DataContract.

[DataContract]
class Item {
   [DataMember]
   public int Data { get; set; }
}

Today, I opened a bug report with an unexpected exception stating that a type named "...Dialog+GuiItem" could not be serialized. Indeed, I could recognize the "Item" part, which was serializable thanks to DataContract, but the GUI component most certainly wasn't! What was going on there?

Looking at the definition of Item, I could see it was a fairly simple type GuiItem augmenting a simple data-type (called Item) with GUI thingies.
class GuiItem : Item {
   public Gui SomeGuiStuff;
}
The problem is, the GUI-augmented type couldn't provide all features the basic type did: It could not be sent over the net.

In the first version of the software, passing the GUI-augmented instance to the middle layer was not a problem. The middle layer expected a basic Item or anything that can be implicitly converted to such an item. As long as all passing of arguments occurs within the same process, no additional effort or processing was needed.
class MidLayer {
   public void DoSomething(Item item);
}
...
void OnClick(GuiItem item) {
  midlayer.DoSomething(item); // Fine, GuiItem is also an Item.
}

In the new version, passing arguments is a significantly more complex operation involving doing a deep dump of the content of the instance, streaming over the net, and summoning a new instance stuffed with the dump on the other side of the wire. This only works for types which are purely data-oriented. Inject any kind of "live" resource in there, and you are out of luck.
[WebService]
class MidLayer {
   [WebMethod]
   public void DoSomething(Item item);
}
...
void OnClick(GuiItem item) {
  midlayer.DoSomething(item); // Exception: can't serialize GuiItem!
}

The solution was simple enough, simply include the basic Item as a member of the GUI-augmented type, and provide a getter to easily send it from the GUI to the server.
class GuiItem {
   Item item;
   Item Item {
      get {
         return item;
      }
   }

   public Gui SomeGuiStuff { get; set; }
}

void OnClick(GuiItem item) {
  midlayer.DoSomething(item.Item); // Fine again.
}

When inheriting from a type, you don't just get its nice features, but also its responsibilities.

2 comments:

Jonathan Allen said...

I think you took the wrong lesson from this. The problem wasn't inheritance, the problem is that GUI logic was being shoved into a class designed to hold data. I see this same mistake occur all the time, even when there is no inheritance involved.

Johann Deneux said...

Jonathan, maybe the very limited view of the code I am giving is misleading, but the application does separate GUI logic and data.

GuiItem couples a GUI component (an entry in a combo box) to a piece of data (an item). The middle layer and the database are completely independent from the Gui, and know nothing of GuiItem. Indeed, we have GUI-less tools to interact with the middle layer and the database.

GuiItem is used as a way to connect the GUI to the data. You can see it as an event handler, if you like. Actually, when presented in that way, it's obvious IS-A was the wrong kind of relation. An invocation of an event handler *has* data describing the event.