Is this a violation of the Liskov Substitution Principle?

Question

Say we have a list of Task entities, and a ProjectTask sub type. Tasks can be closed at any time, except ProjectTasks which cannot be closed once they have a status of Started. The UI should ensure the option to close a started ProjectTask is never available, but some safeguards are present in the domain:

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) 
              throw new Exception("Cannot close a started Project Task");

          base.Close();
     }
}

Now when calling Close() on a Task, there is a chance the call will fail if it is a ProjectTask with the started status, when it wouldn't if it was a base Task. But this is the business requirements. It should fail. Can this be regarded as a violation of the Liskov substitution principle?

score 194 · Accepted Answer · edited Apr 07 '22 at 09:11

Yes, it is a violation of the LSP. Liskov Substitution Principle requires that

Preconditions cannot be strengthened in a subtype.
Postconditions cannot be weakened in a subtype.
Invariants of the supertype must be preserved in a subtype.
History constraint (the "history rule"). Objects are regarded as being modifiable only through their methods (encapsulation). Since subtypes may introduce methods that are not present in the supertype, the introduction of these methods may allow state changes in the subtype that are not permissible in the supertype. The history constraint prohibits this.

Your example breaks the first requirement by strengthening a precondition for calling the Close() method.

You can fix it by bringing the strengthened pre-condition to the top level of the inheritance hierarchy:

public class Task {
    public Status Status { get; set; }
    public virtual bool CanClose() {
        return true;
    }
    public virtual void Close() {
        Status = Status.Closed;
    }
}

By stipulating that a call of Close() is valid only in the state when CanClose() returns true you make the pre-condition apply to the Task as well as to the ProjectTask, fixing the LSP violation:

public class ProjectTask : Task {
    public override bool CanClose() {
        return Status != Status.Started;
    }
    public override void Close() {
        if (!CanClose()) 
            throw new Exception("Cannot close a started Project Task");
        base.Close();
    }
}

score 90 · Answer 2 · edited Jan 13 '23 at 11:56

Yes. This violates LSP.

My suggestion is to add CanClose method/property to base task, so any task can tell if task in this state can be closed. It can also provide reason why. And remove the virtual from Close.

Based on my comment:

public class Task {
    public Status Status { get; private set; }
public virtual bool CanClose(out String reason) {
    reason = null;
    return true;
}
public void Close() {
    String reason;
    if (!CanClose(out reason))
        throw new Exception(reason);

    Status = Status.Closed;
}

}
public class ProjectTask : Task {
    public override bool CanClose(out String reason) {
        if (Status != Status.Started)
        {
            reason = "Cannot close a started Project Task";
            return false;
        }
        return base.CanClose(out reason);
    }
}

Tulains Córdova · Answer 3 · 2016-05-04T15:35:07.920

Liskov substitution principle states that a base class should be replaceable with any of his sub-classes without altering any of the desirable properties of the program. Since only ProjectTask raises an exception when closed, a program would have to be changed to acommodate for that, should ProjectTask be used in substitution of Task. So it is a violation.

But If you modify Task stating in its signature that it may raise an exception when closed, then you would not be violating the principle.

score 26 · Answer 4 · edited Sep 05 '13 at 16:00

An LSP violation requires three parties. The Type T, the Subtype S, and the program P that uses T but is given an instance of S.

Your question has provided T (Task) and S (ProjectTask), but not P. So your question is incomplete and the answer is qualified: If there exists a P that does not expect an exception then, for that P, you have an LSP violation. If every P expects an exception then there is no LSP violation.

However, you do have a SRP violation. The fact that the state of a task can be changed, and the policy that certain tasks in certain states should not be changed to other states, are two very different responsibilities.

Responsibility 1: Represent a task.
Responsibility 2: Implement the policies that change the state of tasks.

These two responsibilities change for different reasons and therefore ought to be in separate classes. Tasks should handle the fact of being a task, and the data associated with a task. TaskStatePolicy should handle the way tasks transition from state to state in a given application.

score 21 · Answer 5 · edited Jul 28 '17 at 13:51

This may or may not be a violation of the LSP.

Seriously. Hear me out.

If you follow the LSP, objects of type ProjectTask must behave as objects of type Task are expected to behave.

The problem with your code is that you have not documented how objects of type Task are expected to behave. You have written code, but no contracts. I'll add a contract for Task.Close. Depending on the contract I add, the code for ProjectTask.Close either does or does not follow the LSP.

Given the following contract for Task.Close, the code for ProjectTask.Close does not follow the LSP:

     // Behaviour: Moves the task to the closed state
     // and does not throw any Exception.
     // Default behaviour: Moves the task to the closed state
     // and does not throw any Exception.
     public virtual void Close()
     {
         Status = Status.Closed;
     }

Given the following contract for Task.Close, the code for ProjectTask.Close does follow the LSP:

     // Behaviour: Moves the task to the closed status if possible.
     // If this is not possible, this method throws an Exception
     // and leaves the status unchanged.
     // Default behaviour: Moves the task to the closed state
     // and does not throw any Exception.
     public virtual void Close()
     {
         Status = Status.Closed;
     }

Methods that may be overridden should be documented in two ways:

The "Behaviour" documents what can be relied on by a client who knows the recipient object is a Task, but doesn't know what class it is a direct instance of. It also tells designers of subclasses which overrides are reasonable and which are not reasonable.
The "Default behaviour" documents what can be relied on by a client who knows that the recipient object is a direct instance of Task (i.e. what you get if you use new Task(). It also tells designers of subclasses what behaviour will be inherited if they don't override the method.

Now the following relations should hold:

If S is a subtype of T, the documented behaviour of S should refine the documented behaviour of T.
If S is a subtype of (or equal to) T, the behaviour of S's code should refine the documented behaviour of T.
If S is a subtype of (or equal to) T, the default behaviour of S should refine the documented behaviour of T.
The actual behaviour of the code for a class should refine its documented default behaviour.

Oswald · Answer 6 · 2012-10-18T20:04:06.363

It is not a violation of the Liskov Substitution Principle.

The Liskov Substitution Principle says:

Let q(x) be a property provable about objects x of type T. Let S be a subtype of T. Type S violates the Liskov Substitution Principle if an object y of type S exists, such that q(y) is not provable.

The reason, why your implementation of the subtype is not a violation of the Liskov Substitution Principle, is quite simple: nothing can be proven about what Task::Close() actually does. Sure, ProjectTask::Close() throws an exception when Status == Status.Started, but so might Status = Status.Closed in Task::Close().

score 5 · Answer 7 · answered Oct 16 '12 at 20:42

5

Yes, it is a violation.

I would suggest you have your hierarchy backwards. If not every Task is closeable, then close() does not belong in Task. Perhaps you want an interface, CloseableTask that all non-ProjectTasks can implement.

answered Oct 16 '12 at 20:42

Thorn G

411

score 3 · Answer 8 · answered Nov 10 '12 at 03:29

In addition to being a LSP issue, it seems like it is using exceptions to control program flow (I have to assume that you catch this trivial exception somewhere and do some custom flow rather than let it crash your app).

It seems like this be a good place to implement the State pattern for TaskState and let the state objects manage the valid transitions.

score 1 · Answer 9 · answered Jul 26 '18 at 17:01

I am missing here an important thing related to LSP and Design by Contract - in preconditions, it is the caller whose responsibility is to make sure the preconditions are met. The called code, in DbC theory, should not verify the precondition. The contract should specify when a task can be closed (e.g. CanClose returns True) and then the calling code should ensure the precondition is met, before it calls Close().

André Ladeira · Answer 10 · 2021-03-07T18:20:13.600

It seems to me that this is an example where the violation of LSP arise, in fact, from a prior violation of some other SOLID principle, in this case, the Single Responsibility.

Let's work with the problem at hand: whats Task's responsibility? Its not clear. Lets say it is to ... perform a Task. If that's the case, it should not worry about whatever close has to do. Clients of, lets say..., AbstractTask interface should only worry about tasks, so every AbstractTask concrete implementation job should be only to implement its task in a predictable and stable way. LSP rules are for that.

On the other hand, there's a requirement of a task always being able to be closed. But there are tasks that can be closed at any time, there are tasks that that must satisfy some conditions before being ready to be closed. This is kinda similar to the canonical LSP violation example of the Real vs Toy duck:

class Duck{
   void quack() = 0 //abstract interface
   ...
}
class RealDuck() {  
   void quack() {// say "quack!"}
   ...
}
class ToyDuck() {  
   void quack() { if(has_batteries) // say quack}
   ...
}

At this point I believe that the best choice depends on your problem requirements. If one expects to have a lot of concrete implementations that violates LSP this way I think that the best choice is to use the Strategy Design Pattern. Strategy decouples the client object from the components that implement its behaviour. This way LSP is intact (no strict requirements in derived classes, change of behaviour chosen at runtime) and greater flexibility and reusability is achieved.

But it doesn't seems to be the case of task/projectTask. In this scenario, where few exceptions to the general rule are expected, a simple and effective solution is to just implement SRP and segregate Closeable from other tasks (e.g., regular tasks implement Closeable, projectTask doesn't) and making clients that may close tasks to rely upon Closeable and not upon Tasks. Static checks at compile time guarantee that projectTask will never be used when freely Closeable objects are required, even if projectTask has a close() method itself.

Sounds good? I hope it helps.

score 1 · Answer 11 · answered Apr 07 '22 at 21:56

You can’t decide whether something is an LSP until you define what a method is supposed to do. And then you check that the baseclass and the derived class match that definition. If the caller relies on behaviour that is not defined, that’s a bug in the caller. But if the behaviour is defined, but one class doesn’t follow it, that’s an LSP violation.

In your case, I can define “Close() either closes the task or throws an exception”. Your two classes don’t violate LSP. Another subclass that quietly ignores Close() calls in the wrong state violates LSP.

I could define Close() in a different way: “Throws an exception in the Started state, otherwise sets the state to closed”. Now Task violates LSP.

There’s the classical example of Rect and Square: It’s just very hard to make Square a subclass of Rect that doesn’t violate LSP.

inf3rno · Answer 12 · 2017-10-10T04:05:11.003

Yes, it is a clear violation of LSP.

Some people argue here that making explicit in the base class that the subclasses can throw exceptions would make this acceptable, but I don't think that is true. No matter what you document in the base class or what abstraction level you move the code to, the preconditions will still be strenghtened in the subclass, because you add the "Cannot close a started Project Task" part to it. This is not something you can solve with a workaround, you need a different model, which does not violate LSP (or we need to loosen on the "preconditions cannot be strenghtened" constraint).

You can try the decorator pattern if you want to avoid LSP violation in this case. It might work, I don't know.

Is this a violation of the Liskov Substitution Principle?

12 Answers12

Linked

Related