7

I am a bit confused as for what it really means. In the related questions (Is this a violation of the Liskov Substitution Principle?), it was said that the example clearly violates LSP.

But I wonder, if there is no new exception thrown, would it still be violation? Isn't it simply polymorphism then? I.e:

public class Task
{
     public Status Status { get; set; }

     public virtual void Close()
     {
         Status = Status.Closed;
     }
}

public class ProjectTask : Task
{
     public override void Close()
     {
          if (Status == Status.Started) 
          {
              base.Close(); 
          }
     }
}
  • It's important to note that in your linked answer, that the LSP violation is incurred as there are NO actual contracts in the base Task Close method, whereas the overridden ProjectTask implementation adds the precondition that status may NOT be Started, i.e. strengthening from No contract to some contract precondition. You don't have any contracts in your example. However there is bad OO practice - by making Close virtual, subclasses can bypass any contracts it may have contained, and by making Status set public, any external code can change state and bypass all contracts and rules anyway. – StuartLC Jan 18 '18 at 14:26
  • @StuartLC Hmm I thought that my contract in the ProjectTask is that the status has to be Started, which is strenghtening from nothing in the base class. – John V Jan 18 '18 at 17:18
5

It depends.

For validating the LSP, you need to know the precise contract of the Close function. If the code looks like this

public class Task
{
     // after a call to this method, the status must become "Closed"
     public virtual void Close()
     //...
}

then a derived class which ignores this comment violates the LSP. If, however, the code looks like this

public class Task
{
     // tries to close the the task 
     // (check Status afterwards to find out if it has worked)
     public virtual void Close()
     //...
}

then ProjectTask does not violate the LSP.

However, in case there is no comment, a function name like Close gives IMHO a the caller a pretty clear expectation of setting the status to "Closed", and if the function then does not work that way, it would be at least a violation of the "Principle of least astonishment".

Note also that some programming languages like Eiffel have in-built language supports for contracts, so one does not necessarily need to rely on comments. See this Wikipedia article for a list.

  • So a solution, in general, would be that the base class would have something like "TryClose" and then the children could implement their own logic. Is that right? – John V Jan 18 '18 at 11:05
  • @user970696: TryClose would probably be a better name, if that is the behaviour you like to model. A comment about the precise expected behaviour in the base class would probably a good idea, too. And if your language supports this, a statement like Postcondition(Status==Status.Started || Status==Status.Closed) in the base class would also indicated that the Status is not necessarily set to "Closed". – Doc Brown Jan 18 '18 at 11:13
  • What I am trying to understand: lets say a base method X immediately outputs "X" to the console. Derived class overrides X and only outputs "X" when the time (seconds) are greater than 10. Is that violation of LSP? – John V Jan 18 '18 at 11:13
  • @user970696: add a contract, and one can tell. A method name like "X" does not serve well as a contract. It does not even serve well for guessing what the intended behaviour should be, – Doc Brown Jan 18 '18 at 11:18
  • To the user of the class, it would imply that X is written in the console. I mean, should it not be enough to just try to substitute the derived class for the base class, to see whether it is working? – John V Jan 18 '18 at 11:21
20

You can't decide whether some code violates the LSP by the code itself. You need to know the contract that each method is to fulfill.

In the example, there is no explicit contract given, so we have to guess what the intended contract of the Close() method might be.

Looking at the base class implementation of te Close() method, the only effect of that method is, that afterwards the Status is Status.Closed. My best guess of a contract for this method reads:

Do whatever is necessary to make the Status become Status.Closed.

But that's just a plausible guess. Nobody can be sure about that if it isn't explicitly written down.

Let's take my guess for granted.

Does the overridden Close() method also fulfill that contract? There are two possibilities that after running this method we have Status.Closed:

  • We already had Status.Closed before calling the method.
  • We had Status.Started. Then we call the base implementation, setting the field to Status.Closed.
  • In all other cases we end up with a different status.

If Status only has the two possible values Closed and Started (e.g. a 2-value enum), everything is fine, there's no LSP violation, because we always get Status.Closed after the Close() method.

But probably there are more possible Status values, ending up in a Status not being Status.Closed, thus violating the contract.


The OP asked about the famous sentence "wherever I am using the base class, its derived class can be used".

So I'd like to elaborate on that.

I read it as "wherever I am using the base class within its contract, its derived class can be used, without violating that contract.

So it's not only about not producing compile errors or running without throwing errors, it's about doing what the contract demands.

And it only applies to situations where I ask the class to do something that's within its intended range of operations. So we need not care about abuse situations (e.g. where preconditions aren't met).


After re-reading your question, I think I should add a paragraph on polymorphism in that context.

Polymorphism means that for instances of different classes, the same method call results in different implementations being run. So polymorphism technically allows you to override our Close() method with one that instead e.g. opens a stream. Technically, that's possible, but it's a bad use of polymorphism. And one principle about good and bad uses of polymorphism is the LSP.

  • Well, but the principle says that wherever I am using the base class, its derived class can be used. So I believe it is possible to see right from the code too. But I hope I understand your point that no expected behavirour is clearly stated. – John V Jan 18 '18 at 11:31
  • @user970696: Well, we suffer a case of insufficient information for deciding that just on the code. As Ralf said, we don't know the definition of Status. Also, we use explicit contracts independent of the code implementing it mainly for two reasons: We might not be able to exhaustively study all instances where our code is used, for example because some of those might not yet be written, or simply be private to someone else. Second, without knowing the contract we cannot determine whether any change to the implementation is just a possible optimization or breaks the world. – Deduplicator Jan 18 '18 at 11:35
  • @user970696: So, LSP was formalized in a 1994 paper by B. Liskov and J. Wing; there they do a couple of things. First they define what subtyping means, and relate it to how a subtype behaves compared to a base type (the paper is called "A behavioral notion of subtyping"), and specify certain constraints that need to be met for this to work. Second they propose some language mechanisms and techniques that would enforce those constraints. However, most OO languages that we use do not have such mechanisms, and this is why it's not enough to just look at the code. (continues below...) – Filip Milovanović Jan 18 '18 at 12:26
  • The contract we speak of defines what is expected of a certain type in terms of behavior. But, since programming languages are generally not expressive enough to encode all aspects of this conctract, we have to rely on documentation, code comments, and other contextual information, as these define the expectations about how the type works. So, for example, if the documentation of a method says "Attempts to [do something]", it's OK for an override to fail. But if it says "Guarantees that [something will happen], and doesn't throw any exception", then it's not, and it's a violation of LSP. – Filip Milovanović Jan 18 '18 at 12:26
  • 2
    @RalfKleberhoff: Unit tests - yeah, but unit test are not a language mechanism (as in, built into the language), that's something we write. And they don't necessarily provide a guarantee, but rather document expectations. Off topic/exceptions: OK, good point - I tried, perhaps a bit clumsily, to illustrate how documentation affects the contract. – Filip Milovanović Jan 18 '18 at 12:57
7

Liskov Substitution Principle is all about contracts. It consists of preconditions (conditions that must hold true so the corresponding behavior could run), postconditions (conditions that must hold true so that behavior could be considered to finish its job), invariants (conditions that must hold true before, during and after the corresponding method execution) and history constraint (in my opinion it's a subset of invariant, so you'd better check wikipedia). In a question you linked to an implied contract of the Task class looks like the following:

  • Precondition: there is none
  • Postcondition: Status is closed
  • Invariant: can't see any

So if one the child classes doesn't close the task, it is considered a violation of LSP within some certain contract.

But if you explicitly postulate your contract to be like "Close the task only if it is started", then you're fine. You can do it in your code -- an example of that is this accepted answer. But very often you can't -- so you could use plain comments.

Basically, every time you think about LSP violation, you should already be familiar with the contract. There is no such thing as just "LSP violation", only "LSP violation within some contract".

  • Right, but now I wonder, isn't polymorphism about implementing different behaviours? – John V Jan 18 '18 at 10:39
  • Only the behaviors that fit the contract. – Zapadlo Jan 18 '18 at 10:43
  • Also,isn't the precondition here the condition Status==Status.Started? Because that represents stronger criteria that conditionally executed the content of the method. – John V Jan 18 '18 at 10:46
  • 1
    @user970696: a precondition is something which must hold before even the method is allowed to be called. So if "Status==Status.Started" was a precondition of Close, calling it with another value for Status would always throw an exception. This is clearly not the case here. – Doc Brown Jan 18 '18 at 11:03
  • 1
    @user970696: that example shows what I wrote: it throws an exception if the precondition is violated. Your example above does not throw any exceptions. – Doc Brown Jan 18 '18 at 11:16
1

Yes, still a violation (probably)

Some client of Task relies on "after Task::Close(), Status is now Closed", and then breaks when it encounters a ProjectTask. You might currently not have any such clients, but then the postcondition of Task::Close() would have to be "Status is in a valid but unspecified state", which is basically pointless.

The much more natural thing is for Task::Close() to have the postcondition "Status is Closed", which precludes the implementation in ProjectTask from being valid.

This is a major problem with void DoStuff() methods: all you have are some side-effects, so you have things relying on those side-effects. bool TryClose() has the meaning "Close() if you can, and tell me about it"

  • Actually, why Task.Close() is the postcondition? – John V Jan 18 '18 at 9:55
  • I am talking about the postcondition of Close – Caleth Jan 18 '18 at 10:08
  • Thanks. So the violation is really in having the condition there? I struggle with differentiating that from polymorphism – John V Jan 18 '18 at 10:10
  • It comes down to what Close means, to the users of Task. If no-one is allowed to care about what it does to Status, then you can do it. It's just that is very limiting, and probably confusing – Caleth Jan 18 '18 at 10:13
  • @user970696 - I guess it would be ok if your method was bool TryClose(). – Emerson Cardoso Jan 18 '18 at 10:23
1

As Ralf and others have mentioned, you haven't actually implemented or enforced any contracts on your code, other than by the assumed "common sense" convention that Close() should leave the object in a closed state, and other than the comments that were added to the subclass.

In my opinion, the example you've provided (I know it's copied from a related post) has a design flaw to declare the Close() method as virtual on the base Task class - this is just inviting others to subclass Task and change the behaviour, even though you've provided a default implementation which observes the contract.

And worse, since Status isn't encapsulated at all, state is publicly mutable, so any contracts around Close are fairly meaningless as state can be randomly assigned externally in any event.

So if your class hierarchy doesn't require polymorphic behaviour of Close, I would simply remove the virtual keyword on Task.Close:

// Encapsulate status, to control state transition
public Status Status { get; private set; }

public void Close()
{
    Status = Status.Closed;
}

(and do the same for any other state transitions)

If however you DO require polymorphic behaviour (i.e. if subclasses need to provide custom implementations of Close), then I would convert your base Task class to an interface, and then enforce any pre and post conditions through Code Contracts, as follows:

[ContractClass(typeof(TaskContracts))]
public interface ITask
{
    Status Status { get; } // No externally accessible set

    void Close();
    // Other transition methods here.
}

With the corresponding contracts:

[ContractClassFor(typeof(ITask))]
public class TaskContracts : ITask
{
    public Status Status { get; }

    public void Close()
    {
        Contract.Requires(Status != Status.Closed, "Already Closed!");
        Contract.Ensures(Status == Status.Closed, "Must close Task on Completion!");
    }
}

The benefit of this approach is that the interface usage contract is clear (and enforceable!), and unlike the virtual Close() which could be bypassed, subclasses can provide any implementation they like, provided that the contract is met.

  • 1
    Your example is technically ok, however, the OP probably wants to have a contract where there is no precondition, and the postcondition is Status==Status.Closed|| Status==Status.Started. – Doc Brown Jan 18 '18 at 11:51
  • The OP's question is clearly about LSP contract violation - the condition, Status == Status.Started is defined in a subclass, which wouldn't have been part of the original contract design, had one been in place. So hypothetically, if the Status.Closed contract had been in place, then yes, OP would have strengthened the post condition, and thus have violated LSP. – StuartLC Jan 18 '18 at 11:56
  • Well mainly I am still puzzled as for what is the precondition. In the examples I found it is often the very first IF in the called method. Yet Doc says it is not the case. – John V Jan 18 '18 at 11:56
  • You haven't defined any contracts, so as it stands, everything is 'fair game', especially since Status has a public setter. If your intention is to enforce DBC, then I would suggest using Code Contracts such as above (your code is C#, right?) and then you will get a compile time warning and a run time error if your ProjectTask subclass doesn't close Status. – StuartLC Jan 18 '18 at 12:03
  • Precondition => object and parameter state going INTO the method. PostCondition => State guarantees leaving the method. Doc is correct, I added the 'Can't already be closed' as an example of a precondition, even though you didn't actually state this as a requirement (and specifically remember that IDisposable.Dispose requires that an object not throw if it has already been Disposed). virtual methods aren't good places for enforcing code contracts, as they can be bypassed altogether in subclasses. – StuartLC Jan 18 '18 at 12:11
0

Yes, it still is a violation of the LSP.

In the base Task class, after Close() has been invoked the Status is Closed.
In the derived ProjectTask class, after invoking Close() the Status may or may not be Closed.

Thus the postcondition (Status is Closed) is no longer fulfilled in the ProjectTask class.
Or in other words: A client only knowing about Task may rely on the fact that Status is Closed after invoking Close(). If you give him a ProjectTask "disguised" as a Task (which you are permitted to do), and he invokes Close() the result is different (Status might not be Closed).

  • Thanks. And what if there was no status change? What if there is still the condition but the only executed command would be a console output or something like that. – John V Jan 18 '18 at 10:29
  • As Ralf Kleberhoff put it so elaborately in his excellent answer: What is the functionality that Close() promises to deliver in the base class (Task). Looking only at the name of the Function (Close()) and its code (set Status to Closed) I infer that (something happens here Status is now Closed) is the promised functionality / postcondition. In that sense, if printing on the console were the promised functionality, then the derived class would need to do that too to uphold the LSP. – CharonX Jan 18 '18 at 13:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.