Design Review – Liskov Substitution Principle

The (Barbara) Liskov Substitution principle states:

If for each object o1 of type S there is an object o2 of type T such that for all programs P defined in terms of T, the behavior of P is unchanged when o1 is substituted for o2 then S is a subtype of T.

That hurts my head.  Let’s again go with the definition from “Agile Principles, Patterns, And Practices in C#” Robert C Martin and Micah Martin:  Subtypes must be substitutable for their base type.

There are two angles from which I want to look at this principle. Let’s look at it first from a structural standpoint.  I’m going to reuse the refactored code from my last post with a slight change.  Rather than having the conditions and actions implement the ISupportRules interface, they will be derived from an abstract class named RuleBase. This means our RulesEngine’s ExecuteRules method needs to accept a generic list of RuleBase.

The Code:

We have our enums :

Code Snippet

2. namespace OpenClosePrinciple
3. {
4.     public enum Actions
5.     {
6.         CreateOrder,
7.         CreateBackorder,
8.         CloseOrder,
9.         ShipOrder,
10.         StoreOrder,
11.         ReduceInventory
12.     }
13.     public enum Conditions
14.     {
15.         IsComplete,
16.         IsInStock,
17.         CanShip
18.     }
19.     public enum RuleType
20.     {
21.         Condition,
22.         Action
23.     }
24. }

An Action derived from RuleBase:

Code Snippet

2. namespace OpenClosePrinciple
3. {
4.     public class CreateOrderAction:RuleBase
5.     {
6.         #region ISupportRules Members
8.         public override bool Execute()
9.         {
10.             return true;
11.         }
13.         public override RuleType TypeOfRule()
14.         {
15.             return RuleType.Action;
16.         }
18.         #endregion
19.     }
20. }

A Condition derived from RuleBase:

Code Snippet

2. namespace OpenClosePrinciple
3. {
4.     public class CanShipCondition:RuleBase
5.     {
6.         #region ISupportRules Members
8.         public override bool Execute()
9.         {
10.             return true;
11.         }
13.         public override RuleType TypeOfRule()
14.         {
15.             return RuleType.Condition;
16.         }
18.         #endregion
19.     }
20. }

RuleBase:

Code Snippet

2. namespace OpenClosePrinciple
3. {
4.     public abstract class RuleBase
5.     {
6.         public virtual bool Execute()
7.         {
8.             return true;
9.         }
11.         public abstract RuleType TypeOfRule();
12.     }
13. }

The Rule Engine.

Code Snippet

1. using System;
2. using System.Data.SqlClient;
3. using System.Collections.Generic;
4. namespace OpenClosePrinciple
5. {
6.     public class RefactoredRuleEngine
7.     {
9.         public void ExecuteRules(List<RuleBase> rules)
10.         {
11.             bool executing = true;
12.             int ruleIndex = 0;
13.             while (executing)
14.             {
15.                 if (rules[ruleIndex].TypeOfRule() == RuleType.Action)
16.                 {
17.                     executing = rules[ruleIndex].Execute();
18.                     ruleIndex++;
19.                 }
20.                 else
21.                 {
22.                     if (rules[ruleIndex].Execute())
23.                     {
24.                         ruleIndex++;
25.                         executing = true;
26.                     }
27.                     else
28.                     {
29.                         ruleIndex += 2;
30.                         executing = true;
31.                     }
32.                 }
33.                 //Implement some exit strategy here
34.             }
36.         }
37.     }
38. }

 

Our Base class simply supports an Execute and a RuleType Method.  Both our condition and Action class derive from RuleBase and may be “passed around” as a RuleBase and therefore we have satisfied the structural idea of being able to substitute a derived class for it’s base class.

The second angle I want to look at this principle is from a behavioral perspective.  Most examples I read demonstrating an LSP violation use the Rectangle base class and the square subclass.   Proof of the LSP violation is based on setting the length and width of the Square to unique values then discovering that an assert in a unit test that calculates a rectangles area returns an incorrect result.  

I agree that this is a violation.  I believe the problem lies in the claim that a square “is-a” rectangle.  Now don’t go running to a dictionary and grab the definition of a rectangle and tell me that a square fits the definition of a rectangle – I’m not talking about the English language. 

The classic implementation of the rectangle when discussing the LSP principle is to expose a Width and a Height property and a CalculateArea method which returns Width X Height.  This makes sense.  The problem in claiming that the Square “is-a” Rectangle is that with a Square there is not a notion of a Width and a Height that are different.  The Width and Height must be the same.  It doesn’t make sense to expose 2 properties, it is misleading and a consumer can logically assume that they would be independent properties.  But in the implementation of the square this is not the case.  Our Square is not (logically) a Rectangle because it does not have a Length and Width that are independent of each other.

To consider whether our design above has violated LSP from a behavior standpoint we have to take a look at the RuleEngine.  At a glance it looks like we have a violation, notice that we have some logic in the engine that concerns itself with the RuleType.

if (rules[ruleIndex].TypeOfRule() == RuleType.Action) …

This does have the “smell” of bad code that we need to constantly look for but I maintain that if we take a closer look this is NOT a violation.  When we cooked up the notion of the RuleEngine we decided we would support 2 types of rules, Conditions and Actions.  It is reasonable to do this.  We would never get our code out the door if we didn’t put some constraints on the types of rules we could support.  The line of code in question is simply executing code based on whether it is a condition or an action.

An example of code that would violate LSP or OCP would be as follows:

if (TypeOf(rules[ruleIndex]) == CanShipCondition)

Here we are trying to execute logic based on the derived type which is a violation of OCP and LSP.  The consumer of the base class would not have to worry about the derived type if the derived type was substitutable for it’s base class and by worrying about derived types, the rules engine is no longer open for extension.

My design review checklist