Posts Tagged 'code smells'

Design Smell: Violating LSP

Sometimes a framework impedes on development so bad, it makes you somewhat angry and feeling helpless. I encountered something today that gives me headaches just thinking about it.

Suppose I have framework in C# 2.0 that models document folder that contains one or more documents. The framework defines an abstract base class, Document:

public abstract class Document {
// Implementation removed.

And concrete document types:

public class PurchaseOrder: Document { ... }
public class Quote : Document { ... }
public class Invoice : Document { ... }

And a DocumentFolder class, the have properties for retrieving each of the concrete document types:

public class DocumentFolder {
  public List<PurchaseOrder> PurchaseOrders { get { ... } }
  public List<Quote> Quotes { get { ... } }
  public List<Invoice> Invoices { get { ... }}

Let’s say I want to collect every PurchaseOrder, Quote, or Invoice document in a DocumentFolder to perform work on them.  Since all these documents derive from Document, I should be able to create a generic list of type Document and use List<T>.AddRange() to collect  them all.

DocumentFolder folder= LoadFolder(id);
List<Document> docs = new List<Document>();
if (folder.PurchaseOrders != null)
if (folder.Quotes != null) docs.AddRange(folder.Quotes);
if (folder.Invoices != null) docs.AddRange(folder.Invoices);

Then, use the List<T>.ForEach() operation to perform common work on
my Documents, using an anonymous delegate:

docs.ForEach(new Action<Document>(
  delegate(Document d) {

But, I get an error upon compiling. After troubleshooting it, I found that the LineItems property is declared as protected internal, and therefore not available to classes outside the executing assembly:

protected internal List<LineItem> LineItems
    /* Implementation removed. */

Each of the three concrete documents override the LineItems property using
the new keyword and calls the base class’ version, effectively crippling
the whole point of having abstract base classes to begin with:

public new List<LineItem> LineItems
  get { return base.LineItems; }


After pounding my head against my computer,  I accepted this design smell and moved on. This example is a  violation of the Liskov Substitution Principle (LSP), which states in a nutshell:

Methods or functions that use references to base classes must be able
to use objects of derived classes without knowing it.

I could not store a list of the base class, populate the list with instances of the derived class, and perform useful iteration. The workaround resulted in three blocks of iterative code that looks almost alike.  So, I will be addressing this problem with my peers in the near future.  In the meantime, back to coding. Later.