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?

Answer 1

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 (Status == Status.Started) 
            throw new Exception("Cannot close a started Project Task");
        base.Close();
    }
}

Answer 2

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 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);
    }
}

Answer 3

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.

Answer 4

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.

Answer 5

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.

Answer 6

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().

Answer 7

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.

Answer 8

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.

Answer 9

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().

Answer 10

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.

Answer 11

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.