PTOM: The Liskov Substitution Principle

The Liskov Substitution Principle

In my first (of hopefully more than one) post for The Los Techies Pablo’s Topic of the Month – March: SOLID Principles effort, I’m going to talk about The Liskov Substitution Principle, as made popular by Robert ‘Uncle Bob’ Martin in The C++ Report.

I’m going to try as much as possible not to repeat everything that Uncle Bob said in the afore-linked PDF as you can go read the important stuff there. I’m going to try to give some real examples and relate this to the .NET world.

In case you’re too lazy to read the link, let me start off with a quick summary of what LSP is: If you have a base class BASE and subclasses SUB1 and SUB2, the rest of your code should always refer to BASE and NOT SUB1 and SUB2.

A case study in LSP ignorance

The problems that LSP solves are almost always easily avoidable. There are some usual tell-tale signs that an LSP-violation is coming up in your code. Here’s a scenario that walks through how an LSP-violation might occur. I’m sure we’ve all run into situations like this. Hopefully by walking through this, you can start getting used to spotting this trend up front and cutting it off before you paint yourself into a corner.

Let’s say that somewhere in your data access code you had a nifty method through which all your DAO’s/Entities passed and it did common things like setting the CreatedDate/UpdatedDate, etc.

public void SaveEntity(IEntity entity)
{
DateTime saveDate = DateTime.Now;

if( entity.IsNew )
entity.CreatedDate = saveDate;

entity.UpdatedDate = saveDate;

DBConnection.Save(entity);
}

Clever. Works like a champ.  Many of you will hopefully have cringed at this code. I had a hard time writing it, but it’s for illustration. There’s a lot of code out there being written like this.  If you didn’t cringe and you don’t see what’s wrong with that code, please continue reading. Now, the stakeholders come to you with a feature request:

Whenever a user saves a Widget, we need to generate a Widget Audit record in the database for tracking later.

You might be tempted to add it to your handy-dandy SaveEntity routine through which all entities pass:

public void SaveEntity(IEntity entity)
{
WidgetEntity widget = entity as WidgetEntity;
if( widget != null )
{
GenerateWidgetAuditEntry(widget);
}

// ...

 

Great! Also works like a champ. But a few weeks later, they come to you with a list of 6 other entities that need similar auditing features.  So you plug in those 6 entities. A few weeks later, the come to you and ask you something like this:

When an Approval record is saved, we need to verify that the Approval is of the correct level. If it’s not the correct level, we need to prompt the user for an excuse, otherwise they can’t continue saving.

Oh boy, that’s tricky. Well, now our SaveEntity looks something like this:

public void SaveEntity(IEntity entity)
{
if( (entity as WidgetEntity) != null ){
GenerateWidgetAuditEntry((WidgetEntity) entity);
}

if ((entity as ChocolateEntity) != null){
GenerateChocolateAuditEntry((ChocolateEntity)entity);
}

// ...

ApprovalEntity approval = entity as ApprovalEntity;
if( approval != null && approval.Level < 2 ){
throw new RequiresApprovalException(approval);
}

// ...

 

Pretty soon your small, clever SaveEntity method is 1,500 lines long and knows everything about every entity in the entire system.

Where’d we go wrong?

Well, there’s several places to start here. Centralizing the saving of entities isn’t the greatest idea.  Putting the logic for whether audit entries need created or not into the SaveEntity method was definitely the wrong thing to do.  And, finally, due to the complexities of handling wildly differing business logic for different entities, you have a control flow problem with the approval level that requires the use of an thrown exception to break out of the flow (which is akin to a ‘goto’ statement in days of yore).

The concerns of auditing, setting created/updated dates, and approval levels are separate and orthogonal from each other and shouldn’t be seen together, hanging around in the same method, generally making a mess of things. 

But, more to the point of this blog post: SaveEntity violates the Liskov Substitution Principle.  That is to say, SaveEntity takes an IEntity interface/base class but deals with specific sub-classes and implementations of IEntity. This violates a fundamental rule of object-oriented design (polymorphism) since SaveEntity pretends to work with any particular IEntity implementation when, in fact, it doesn’t. More precisely, it doesn’t treat all IEntity’s exactly the same. Some get more attention than others.

Why is this a problem? What if you were reusing your terribly clever SaveEntity method on another project and have dozens of IEntity implementations over there and the stakeholders for that project also wanted the auditing feature. Now you’ve got a problem.

Solutions

One fine approach to this problem of having to do things a-the-moment-of-saving would be to use the Visitor Pattern as described by Matthew Cory in this post.  Though, I would say in this particular example, there is a much more deep-rooted and systemic design problem which revolves around the centralization of data access.

Another, in our case more preferable, way to go might be to use the repository pattern for managing data access.  Rather than having “One Method to Rule them All”, you could have your repositories worry about the Created/Updated date time and devise a system whereby all the repository implementations share some of the Created/Updated date entity save/setup logic.

As specific one-off problems arise (such as auditing, extra approval/verification,etc) they can be handled in a similarly one-off manner by the individual entity’s related repository (who knows all about that one type of entity and that’s it).  If you notice that several entities are doing the same sort of thing (i.e. auditing) you can, again, create a class and method some where for handling auditing in a common manner and providing the various repositories who need auditing with that functionality.  Resist, if at all possible, the urge to create an ‘AuditingRepositoryBase’ class that provides the auditing functionality. Inevitably, one of those audit-requiring entities will have another, orthogonal concern for which you will have another *Base class and, since you can’t do multiple inheritance in .NET, you are now stuck.  Prefer composition of functionality over inheritance of functionality whenever possible.

If you have a rich domain model, perhaps the most elegant approach of all would be to make things like auditing a first-class feature of the domain model whereas every Widget always has at least one WidgetAuditEntry associated with it and this association is managed through the domain logic itself.  Likewise, the approval level would be best handled higher up in the logic chain to prevent last minute “gotchas” in the lifecycle that would require something less than elegant like an exception as a thinly veiled ‘goto’ bailout.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Chad Myers

Chad Myers is the Director of Development for Dovetail Software, in Austin, TX, where he leads a premiere software team building complex enterprise software products. Chad is a .NET software developer specializing in enterprise software designs and architectures. He has over 12 years of software development experience and a proven track record of Agile, test-driven project leadership using both Microsoft and open source tools. He is a community leader who speaks at the Austin .NET User's Group, the ADNUG Code Camp, and participates in various development communities and open source projects.
This entry was posted in .NET, Principles, Programming, PTOM. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.derickbailey.com Derick Bailey

    LSP = Polymorphism… one of the three basic tenets of OOP.

  • http://www.codethinked.com Justin Etheredge

    I hate to sound like I am nit-picking, but the LSP doesn’t really mean that you should always use the base class. It means that anywhere that the base class is expected you should be able to substitute any child class and they will operate as if they were the base class.
    I am not saying whether your advice is right or wrong, I am just saying that by the letter the LSP says “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.”

  • http://chadmyers.lostechies.com Chad Myers

    @Justin:
    I’m not sure how what you said is any different than what I said in my post. I was saying that taking an IEntity parameter to a method and then casting it to FooEntity and BarEntity is wrong (i.e. you’re not treating each IEntity subclass the same).

    Where do we differ here, I’m not sure?

  • http://www.OdeToCode.com/blogs/scott/ Scott Allen

    I think where you and Justin are differing is that LSP is more than just treating each IEntity subclass the same. The SaveEntity method is bad, I agree, but there’s another side of the LSP coin – be careful when you are subclassing and overriding behavior in the derived entities.

    Your entities in this case only seem to have two properties, so a contrived example would be to imagine writing a test like:

    entity.UpdateDate = saveDate;
    Assert.AreEqual(entity.UpdateDate, saveDate);

    Say this Assert works for a WidgetEntity, because the WidgetEntity always publishes it’s last update time through the UpdateDate property (it’s hard to talk about LSP without talking about preconditions and postconditions) – that’s the behavior all client’s of WidgetEntity expect to observe.

    The Assert should then also work for any class derived from WidgetEntity.

    Imagine that some jerk derives a class from WidgetEntity called CompositeWidgetEntity, but only passes a new UpdateDate value along to it’s collection of child entities that it manages. It doesn’t actually remember an update time – you have to get the UpdateDate from it’s child objects.

    The Assert will fail, LSP is violated, and CompositeWidgetEntitiy shouldn’t derive from WidgetEntity.

    That example is a bit contrived – most LSP violations are a bit more subtle. You generally have to think more about LSP when subclassing or overriding a virtual than you do when implementing a method like SaveEntity. It’s easier to spot dumb code in SaveEntity, it’s harder to see and understand an inheritance hierarchy.

  • http://www.OdeToCode.com/blogs/scott/ Scott Allen

    A simpler example, now that I think about it, is a ReadOnlyWidgetEntity that derives from WidgetEntity and throws an exception if you set UpdateDate. That’s a violation that you could solve using typeof in SaveEntity() – but the real problem is in the inherentince hierarchy. I can’t substitute a ReadOnlyWidgetEntity for a WidgetEntity.

  • http://www.elegantcode.com Jarod

    I started busting this out to show how someone might solve the LSP issue shown in your orginal example… I didnt take it too far, but hopefully someone can get the idea. Thoughts?

    //Generic & entensible now without violating LSP
    public void SaveEntity(IEntity entity)
    {
    entity.Validate();
    entity.Audit();

    DBConnection.Save(entity);
    }

    //add methods to audit and validate business rules
    public interface IEntity
    {
    bool IsNew { get; set; }
    DateTime CreatedDate { get; set; }
    DateTime UpdatedDate { get; set; }
    void Audit();
    void Validate();
    }

    public class WidgetEntity : IEntity
    {
    private IAuditor _auditor;
    public WidgetEntity(IAuditor auditor)
    {
    //Should inject this
    _auditor = auditor;
    }

    #region IEntity Members

    public bool IsNew { get; set; }
    public DateTime CreatedDate { get; set; }
    public DateTime UpdatedDate { get; set; }

    public void Audit()
    {
    _auditor.Audit(this);
    }

    //this could go alot further, perhaps a vistor
    public void Validate()
    {
    //run approval or any business rules here
    }

    #endregion
    }

    public class DefaultAuditor : IAuditor
    {
    #region IAuditor Members

    public void Audit(IEntity entity)
    {
    var saveDate = DateTime.Now;
    if (entity.IsNew)
    entity.CreatedDate = saveDate;
    entity.UpdatedDate = saveDate;
    }

    #endregion
    }

    public interface IAuditor
    {
    void Audit(IEntity entity);
    }

  • http://www.potschka-it.de Florian Potschka

    @Jarod:
    That´s exactly the same I would have done in this case. With this solution SaveEntity only depends on IEntity and not on its special implementations. I think thats the standard OOP-way of handling such problems. Simple and working. I think using designpatterns in this case would be overkill.

    @Chad:
    Nice written post. But I agree with my previous speakers, that the presented example is not really a perfect fit. What you described maps better to the Open-Closed-Principle (OCP): Classes should be open for extension but closed for modification. For further info see http://www.objectmentor.com/resources/articles/ocp.pdf.

    LSP is a special case of the OCP. Every time you violate the LSP, you violate the OCP as a result. But not the other way round.

    Perhaps you can find a better example for your blog-post. I think its an very important OOP-principle and you express yourself very clear and understandable.

  • http://www.codethinked.com Justin Etheredge

    @chadmyers In your post you said this “If you have a base class BASE and subclasses SUB1 and SUB2, the rest of your code should always refer to BASE and NOT SUB1 and SUB2.” I took that as you saying that I should *always* refer to “BASE” and not “SUB1″ or “SUB2″. But this really doesn’t have anything to do with LSP. LSP doesn’t talk about not referring to child classes, it talks about what happens if you substitute a child class for a base class.
    Again, I’m not saying your advice is wrong, I’m just saying that your definition of LSP is stretched a tiny bit.

  • http://chadmyers.lostechies.com Chad Myers

    @Justin:

    You’re right, that statement isn’t accurate. I didn’t mean to say that all code should reference BASE, I meant that any code already referencing BASE, should not have to know or otherwise worry about SUB1 or SUB2 or, indeed, any other subclass thereof.

  • http://chadmyers.lostechies.com Chad Myers

    @Scott Allen:

    Yeah, the example is not perfect, but honestly this was probably the 6th attempt at an example. It turns out that LSP is so easily spottable from 10 miles away, that I couldn’t even force myself to write the backwards code necessary to create an LSP violation. This is the best I could do, ha!

    @Florian:

    I agree with your LSP is OCP statement, but I’m not sure what you’re suggesting I do. If you can’t violate LSP without also violating OCP, then how could I ever contrive a scenario that didn’t violate both?

  • http://chadmyers.lostechies.com Chad Myers

    @Jarod:

    Not bad, but I would say that you should not have a centralized ‘SaveEntity’ type method because eventually you’re going to get into a case that you simply cannot treat all IEntity’s the same (there will eventually be some entities that require something that cannot be boiled down to a simple interface).

    In your example, you’ve now violated SRP because you’ve added an Auditing concern to IEntity. What happens when you need logging? versioning? More concerns will definitely come up and you can’t just keep adding them to IEntity.

    That’s why I suggested going to a Repository model where you have a repository for every type of IEntity (or maybe a group of IEntity’s). These Repositories would know about the specific entit(y|ies) they’re responsible for and how to handle the specifics of dealing with that entity (i.e. auditing, logging, etc)

  • http://www.potschka-it.de Florian Potschka

    I didn´t meant you should reach for an example where only LSP is violated. The problem is, that your example is better suited to demonstrate the OCP than the LSP. I think it´s pretty hard to come up with a really good example. Perhaps this is the reason why Robert Martin has chosen the simple ellipsis-and-circle-example.

    I´m not really sure, that I understand how the repository-pattern would be applied in this situation. Wouldn´t you only move responsibilities from the Entity to an external handler? Perhaps you can give a short example …

  • http://haacked.com/ Haacked

    Here’s one example of an LSP violation.

    public class Foo
    {
    public virtual void DoThis()
    {
    if(isError)
    throw new MyException();
    //Do great stuff
    }
    }

    public class Bar
    {
    public override void DoThis()
    {
    if(isError)
    throw new DifferentException();
    //Do great stuff
    }
    }

    Now suppose we have another class with the following method…

    public void DoSomething(Foo foo)
    {
    try
    {
    foo.DoThis();
    }
    catch(MyException e)
    {
    }
    }

    The method Bar.DoThis causes the class Bar to violate LSP because it cannot be substituted for Foo, because Bar.DoThis() throws a different exception type.

    One Fix in this case is if DifferentException inherits from MyException. In that case, you could substitute Bar for Foo anywhere you see a method that accepts Foo.

    phil

  • http://haacked.com/ Haacked

    p.s.

    I think the difficulty in your example is you chose to use an interface to demonstrate LSP. An interface doesn’t specify any pre/post conditions.

    Thus if you implement an interface, how would you know if you violate LSP? Well you’d have to know how every method that accepts that interface is implemented. That’s too much info.

    Likewise, if you write a method that accepts the interface, how would you know that your method doesn’t use the interface in such a way that LSP is violated? Well you’d have to know about every implementation of that LSP.

    Applying LSP makes more sense when you’re talking about one class inheriting from another.

    So in your example, you have an interface, a class that implements that interface, and then yet another sub class. That subclass would be the thing that might violate LSP.

  • http://www.potschka-it.de Florian Potschka

    @Haacked:
    Good example! You are right: The LSP can be best demonstrated with a given context. You learn something new every day :-)

    p.s. Bar should extend Foo.

  • http://www.elegantcode.com Jarod

    @Chad
    First, a great post allowing some excellent discussion!

    Second, In your original post you show the wrong way to do it, without ever coming back and showing the right, or corrected way, which as I reader I would have like to have seen. (vs some links pointing to ideas)

    In all of your code examples the point is the SaveEntity violating LSP, I was just working with your example. I would in fact reccomend the repository, however, In my repository I would still use some sort of abstraction in Save that does common stuff like Audit, Validate etc.

    @Haaccked
    Good point

  • http://chadmyers.lostechies.com Chad Myers

    @Haacked in terms of inheritance, the interfaces make no difference. Perhaps you’ve got ABS on the brain lately? ;)

    In fact, I would argue that interfaces put an exclamation point on the whole LSP demonstration.

    Second, The rest of you (@Haacked, @Florian) seem to be talking about a different LSP than I am. Here’s the quintessential example of LSP violation:

    B = base class
    S1 = Subclass 1 : B
    S2 = Subclass 2 : B

    Sn = Subclass n : B

    public void WorkOnSomething( B b )
    {
    //… bunch of code working on ‘b’

    if( b is S1 )
    {
    DoSomethingFancyWithS1( (S1) b );
    }
    else if( b is S2 )
    {
    DoFancierThingWithS2( (S2) b );
    }
    else if( b is Sn )
    {
    UnbelievablyCoolThingWithSn( (Sn) b );
    }
    else
    {
    throw new InvalidOperationException(“Don’t know how to process type ” + b.GetType().Name);
    }
    }

    And yes, I’ve seen code exactly like this. Any time you extended type B, you had to go around and find all these if/elseif and switch statements all around the code.

    When I think LSP murder, that’s what I think of.

    @Haacked: I can kinda agree with the throws-exception-variance-is-LSP-violation argument, it’s sketchy at best. Can you think of another example that might make it clearer to me what you’re trying to illustrate? Sorry to put the onus back on you, but I want to make sure I understand where you’re coming from.

    @Jarod: Well, I didn’t want to make the post any longer than it was already, but I also didn’t want to post a ‘recommended’ solution that people were going to run with and later come back and say “But you told us this was the right way!”

    I merely wanted to point out what you need to avoid. There are a million RIGHT ways of solving that problem.

    That said, I’ll post a heavily disclaimer’d “Here’s how I might go about solving this problem” post soon

  • http://chadmyers.lostechies.com Chad Myers

    @Florian When I said Repository pattern, I was referring more to the Evans version than the Fowler version, but they’re not so different that you couldn’t bend it either way.

    At any case, if you had a WidgetEntity that required special one-off save logic, you would have a WidgetRepository that was registered (with IoC?) to handle saving for that WidgetEntity. Anyone wishing to persist a WidgetEntity would use a WidgetRepository and it would handle things like audit logging, etc.

    In general, having a centralized force-inheritance model for saving entities is virtually a guaranteed recipe for LSP violation which is why you should de-centralize the entity logic and use composition to bring in the common bits of functionality.

  • http://www.potschka-it.de Florian Potschka

    Hm, this is exactly an example that I would map to the OCP, because WorkOnSomething() is not closed for modification. The LSP is only violated along the way.

    Imho your statement “And yes, I’ve seen code exactly like this. Any time you extended type B, you had to go around and find all these if/elseif and switch statements all around the code. When I think LSP murder, that’s what I think of.” is the perfect explanation of the OCP.

    But perhaps these are just different ways of looking on the same problem. OCP and LSP are somewhat difficult to seperate. It always depends on what is in your focus.

    As to the repository pattern: Wouldn´t the WidgetRepository violate the SRP too, if you add logging, auditing and versioning-responsibilities to it?

  • http://www.potschka-it.de Florian Potschka

    I thought about it for a while… Even though the WidgetRepository violates the SRP too, this is unavoidable. For complex concerns the repository seems to be a nice solution.

  • http://chadmyers.lostechies.com Chad Myers

    @Florian:

    Eventually some coordination has to happen somewhere to make all these things happens. I suppose you could push it into configuration or IoC somehow but then you lose some visibility (which may not be a bad thing, I guess).

    As long as the details of the extra responsibilities are hidden from the callers and the specific functionality for actually writing audit entries is perhaps abstracted into an interface which is injected via IoC into the repository, I think you have a very minimal SRP violation if at all and you can test the Widget repo in isolation without too many mocks

  • http://haacked.com/ Haacked

    Hey Chad, here’s the original quote by Barbara Liskov.

    “What is wanted here is something like the following substitution property: 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.”

    Read that carefully and you’ll see what I mean.

    In my example (yes, Bar should extend Foo. Typo), the class with the method DoSomething represents P. Foo represents T. Bar represents S.

    LSP is a *strong* definition of subtype. Bar inheriting Foo doesn’t mean that Bar is a “subtype” of Foo according to LSP. That becomes apparent as I demonstrated how LSP can be violated.

  • http://chadmyers.lostechies.com chadmyers

    @Haacked, @Florian:

    I see what you’re saying now. I had been focusing too much on LSP and when I looked at the OCP paper again, I see now what you’re saying.

    I don’t think my example, while not very precise, is harmful so I think I’ll keep this post up, but it’s more about OCP and less about LSP (in fact, very little about LSP).

    Maybe this might be a way of describing what you’re all telling me:

    While checking for specific types is technically a violation of LSP, it’s more commonly thought of as an OCP violation. LSP is more focused on the behavior of the specific instance itself, rather than types operating upon the specific instance.

    @Haacked: I see how your example makes more sense, but I’m still not crazy about the exception idea.

    I think I may have a better LSP violation example, how’s this:

    public interface ICalculator
    {
    bool TryDivide(double operand1, double operand2, out double result);
    }

    // A good implementation
    public class StandardCalculator : ICalculator
    {
    public bool TryDivide(double operand1, double operand2, out double result)
    {
    result = 0;

    if( operand2 == 0 ) return false;

    result = operand1 / operand2;

    return true;
    }
    }

    // LSP-violating implementation
    public class BADCalculator : ICalculator
    {
    public bool TryDivide(double operand1, double operand2, out double result)
    {
    result = 0;

    if( operand2 == 0 ) throw new DivideByZeroException();

    result = operand1 / operand2;

    return true;
    }
    }

    As you can see, BADImplementation clearly violates the intended/express behavior of how TryDivide is supposed to work.

    What do ya’ll think?

  • http://haacked.com/ Haacked

    @chad exactly! :) The exception example is an easy way to express an LSP violation because the exception a method throws is part of its “contract”.

    Here’s another that doesn’t rely on an exception. Say you have a class that accumulates matter.

    public class MatterAccumulator
    {
    public int Amount {get; private set;}
    public virtual int Accrete(int delta)
    {
    Amount += Math.Abs(delta);
    }
    }

    And another class that uses it assumes that every call to accrete increases the size.

    public class Something
    {
    public int Execute(MatterAccumulator accumulator)
    {
    while(accumulator.Count < 1000)
    {
    accumulator.Accrete(10);
    }
    }
    }

    Now suppose you wrote a different kind of accumulator…

    public AntiMatterAccumulator : MatterAccumulator
    {
    public override int Accrete(int delta)
    {
    Amount += -1 * Math.Abs(delta);
    }
    }

    Now if you substitute AntiMatterAccumulator for MatterAccumulator, you’ll get an infinite loop in the Execute method (yeah, this is kind a cheesy). Thus AntiMatterAccumulator violates LSP.

    This is kinda fun trying to purposely violate a princple as a way of internalizing it. :)

  • http://www.potschka-it.de Florian Potschka

    @chad:

    “While checking for specific types is technically a violation of LSP, it’s more commonly thought of as an OCP violation. LSP is more focused on the behavior of the specific instance itself, rather than types operating upon the specific instance.”

    I could not have said this better myself. :-)

    @chad, @haacked, @Jarod:

    Great examples and a very fruitful discussion!

  • Per Erik Stendahl

    What’s your point? The original SaveEntity() does it’s job well and looks ok (I didn’t cringe :-). Then, along the way, specifications change and you need to add more stuff to SaveEntity() until it’s evolved to some ugly behemoth and needs to be refactored. Standard software development practices, right?

    Question is: how do we refactor SaveEntity()? If we control IEntity we could add the required interfaces to it although that could result in lots of versioning and testing issues. If we don’t control IEntity or don’t want to change it we could implement some kind of Dependency Injection in SaveEntity(). If we used a language that supports polymorphism on parameters that would probably be a good idea. Maybe extension methods can be used? So many questeions…. :-)

  • http://chadmyers.lostechies.com Chad Myers

    @Erik:

    The problem with SaveEntity that it increases the effort necessary to add a new type of entity. Also, should you ever wish to reuse this code for another type of project, you’re SOL.

    Let me turn the question back on you (anyone): What problem is SaveEntity solving and why does anyone think solves it well?

    If the goal is merely to have a single place in code where the Created/UpdatedDates get set, I could think of several better places to put that without necessarily having to bottleneck the entire inheritance hierarchy of my domain.

  • Per Erik Stendahl

    @chad: Do you mean the first version of SaveEntity() or the last? The Created/Updated properties could probably have been implemented smarter but the problem, as I see it, is that we have an operation, SaveEntity(), that’s external to IEntity, and although the first version probably does exactly what’s needed, the specs change and we suddenly need to adapt SaveEntity() to the specific instance class. Changes to the specification happens all the time and is a part of life I think (and thus unavoidable, unless you want to quit :). How would you avoid this situation?

    I’m always looking for better ways to design my software but trying to foresee every possible specification change? I don’t think I will ever be able to do that. :-)

    (keep trying though)

  • http://colinjack.blogspot.com Colin Jack

    @Chad
    “In case you’re too lazy to read the link, let me start off with a quick summary of what LSP is: If you have a base class BASE and subclasses SUB1 and SUB2, the rest of your code should always refer to BASE and NOT SUB1 and SUB2.”

    I’m not really sure thats what LSP is about at all. It’s about making all classes in a hierarchy (or under and abstraction) behave consistently according to a predefined contract. Your example is thus, to me, not really related to LSP as such.

    A good example is the way the readonly collection stuff works in the .NET framework, how it wraps your collection and raises an exception. Vomit!

    Oh just read the comments, I’m too late to this discussion :)

    @Chad
    “That’s why I suggested going to a Repository model where you have a repository for every type of IEntity (or maybe a group of IEntity’s). These Repositories would know about the specific entit(y|ies) they’re responsible for and how to handle the specifics of dealing with that entity (i.e. auditing, logging, etc)”

    Sounds sensible but UOW interferes and more importantly your repository isn’t necessarily involved enough in the object to allow you to audit, especially as you may well be auditing domain events that the repository will not be involved in (e.g. state changes not just save/load). Depends on the situation but I’d say AOP is a better option.

    @Haacked
    “The exception example is an easy way to express an LSP violation because the exception a method throws is part of its “contract”.”

    Definitely, but then why do we have read only wrappers for collections in .NET that just raise exceptions :(

    Validation is different again, thats truly a domain concern.

    @Haacked
    “Likewise, if you write a method that accepts the interface, how would you know that your method doesn’t use the interface in such a way that LSP is violated? Well you’d have to know about every implementation of that LSP.”

    In some cases its impossible anyway though (http://www.c2.com/cgi/wiki?LiskovSubstitutionPrinciple).

    @Haacked
    “So in your example, you have an interface, a class that implements that interface, and then yet another sub class. That subclass would be the thing that might violate LSP.”

    I get you. You can’t enforce pre/post conditions at the interface level but you can make it clear what the expected behavior of subclasses is.

  • http://www.flux88.com Ben Scheirman

    Here’s a really simple, but excellent example of a clear violation of LSP:

    public class Rectangle
    {
    public int Width { get; set; }
    public int Height { get; set; }
    public int Area
    {
    get { return Width * Height;}
    }
    }

    public class Square : Rectangle
    {
    public int Width { get; set { this.Width = value; this.Height=value; }
    public int Height { get; set { this.Width = value;
    this.Height = value; }
    }

    Here’s a test that fails:

    [Test]
    public void CanCalcArea()
    {
    Rectangle rect = new Square();
    rect.Width = 5;
    rect.Height = 2;
    Assert.That(rect.Area, Is.EqualTo(10));
    //FAILS! result is 4
    }

    Square clearly IS-A rectangle, but also violates LSP. You can’t treat square the same as rectangle.

    So when your methods accept a class of type A, it must also be able to treat all derivatives of A the same way.

    Good stuff Chad, I wish more people thought about these things. :)

  • http://chocolatefordogs.com Matthew Cory

    Chad,
    Thanks for the link. I’m coming a bit late in the ball game here, so I only skimmed over the comments, but there were some that kind of stood out — most specifically by Haacked — that I wanted to touch on briefly. Nothing really new, just wanted to see if I could help to murk up the waters around here a little.

    I’m probably wrong on this — I don’t claim to really know what I’m talking about, just enough to fake it a little bit — but it seems that there’s almost two LSP’s, one that’s based more on methods and hierarchy, and one that’s based more on contracts. The one I addressed in the post you linked to (and a couple of other posts as well) is more in line with the hierarchal violations — and it seems that’s the one your post here focused on.

    Those are really easy to spot — look for a type cast, and you’ve found one. Something like:
    ICollection col = CollectionFactory.Build();
    if (col is List) (col as List).Add(value);
    else if (col is Dictionary
    ) (col as Dictionary).Add(key, value);

    The ones that Haacked was referring to — and that Ben pointed out in the last comment — are the contractual ones. Basically, you have a method called “int AddOne(int val)” that returns whatever value you pass to it incremented by one; in a descendant class, you override that method and have it add two instead of one. That is also an LSP violation, just a lot more subtle and a lot more difficult to find.

    I’d almost go so far as to say that the contractual violations are the *real* LSP violations, while the hierarchal violations are actually a bit closer to a DIP violation; still not quite right, but it might be better to break them apart somehow.

    Anyways — thanks again for the link.

  • http://www.bluespire.com/blogs Christopher Bennage

    Jumping back to Haacked’s Foo and Bar example.

    Is the violation in Bar? or rather in the fact that Foo allows the method to be overridden? Could we build Foo in such a defensive way to prevent violation? (Probably not without over engineering).

    What if Foo is not my code, and I don’t really know what the virtual method does? How did I implement Bar?

    I could read Barbara’s statement such the responsibility of enforcing the principle lies with the consumer of Foo and Bar. (Not depending on any virtual methods of Foo). Maybe that’s stretching it.

    All of this begins to feel like the Principle of Least Surprise.