Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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 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?

share|improve this question
5  
Perfect to a T example of violating liskov substitution. Do not use inheritance here, and you'll be fine. –  Jimmy Hoffa Oct 16 '12 at 20:39
7  
You might want to change it to: public Status Status { get; private set; }; otherwise the Close() method can be worked around. –  Job Oct 16 '12 at 20:52
2  
Maybe it's just this example, but I see no material benefit to complying with the LSP. To me, this solution in the question is clearer, easier to understand, and easier to maintain than one the complies with LSP. –  Ben Lee Oct 22 '12 at 20:21

7 Answers 7

up vote 103 down vote accepted

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 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();
    }
}
share|improve this answer
8  
Great detail and great solution +1 –  Jimmy Hoffa Oct 16 '12 at 20:51
5  
I don't like duplication of that check. I would prefer exception throwing going into Task.Close and remove virtual from Close. –  Euphoric Oct 16 '12 at 20:53
3  
@Euphoric: But now there is no way of answering the question, "Can this task be closed?" without trying to close it. This unnecessarily forces the use of exceptions for flow control. I will admit, however, that this kind of thing can be taken too far. Taken too far, this kind of solution can end up yielding an an enterprisy mess. Regardless, the OP's question strikes me as more about principles, so an ivory tower answer is very much appropriate. +1 –  Brian Oct 16 '12 at 20:58
12  
@Brian The CanClose is still there. It can still be called to check if Task can be closed. The check in Close should call this too. –  Euphoric Oct 16 '12 at 20:59
2  
@Euphoric: Ah, I misunderstood. You're right, that makes for a much cleaner solution. –  Brian Oct 16 '12 at 21:04

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);
    }
}
share|improve this answer
2  
Thanks for this, you took dasblinkenlight's example one stage further, but I did like his explanationa nd justification. Sorry I can't accept 2 answers! –  Paul T Davies Oct 17 '12 at 7:55
2  
this should be higher rated, as the implementation is definitely better! –  SwissCoder Oct 17 '12 at 9:13
    
I'm interested to know why the the signature is public virtual bool CanClose(out String reason) -- by using out are you merely future-proofing? Or is there something more subtle that I'm missing? –  Reacher Gilt Oct 18 '12 at 20:51
    
@ReacherGilt What? No. The override is using it. Yes, you might return reason or null, but that would require client making the check. –  Euphoric Oct 19 '12 at 5:36
1  
@ReacherGilt I think you should check what out/ref do and read my code again. You are confused. Simply "If task cannot close, I want to know why." –  Euphoric Oct 19 '12 at 17:46

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 can raise an exception when closed, then you would not be violating the principle.

share|improve this answer
1  
+1 for adding the Exception possibility to Task –  Matthew Flynn Oct 16 '12 at 21:29
    
I use c# which I don't think has this possibility, but I know Java does. –  Paul T Davies Oct 17 '12 at 7:56
1  
@PaulTDavies You can decorate a method with what exceptions it throws, msdn.microsoft.com/en-us/library/5ast78ax.aspx . You notice this when you hover over a method from the base class library you will get a list of exceptions. It is not enforced, but it makes the caller aware nonetheless. –  Despertar Jul 19 '13 at 6:19

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.

share|improve this answer
    
Responsibilities heavily depend on domain and (in this example) how complex task states and its changers are. In this case, there is no indication of such thing, so there is no problem with SRP. As for the LSP violation, I believe we all assumed that caller doesn't expect an exception and application should show reasonable message instead of getting into erroneous state. –  Euphoric Sep 4 '13 at 16:40
    
Unca' Bob responds? "We're not worthy! We're not worthy!". Anyway... If every P expects an exception then there is no LSP violation. BUT if we stipulate a T instance cannot throw an OpenTaskException (hint, hint) and every P expects an exception then what does that say about code to interface, not implementation? What am I talking about? I don't know. I'm just jazzed that I'm commenting on an Unca' Bob answer. –  radarbob Sep 5 '13 at 16:32

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.

share|improve this answer
1  
Every Task is closable, but not under every circumstance. –  Paul T Davies Oct 16 '12 at 20:46
    
This approach seems risky to me as people may write code expecting all Task's to implement ClosableTask, though it does accurately model the problem. I'm torn between this approach and a state machine because I hate state machines. –  Jimmy Hoffa Oct 16 '12 at 20:47
    
If Task doesn't itself implement CloseableTask then they're doing an unsafe cast somewhere to even call Close(). –  Tom G Oct 16 '12 at 20:49
    
@TomG that's what I'm afraid of –  Jimmy Hoffa Oct 16 '12 at 20:52
    
There is already a state machine. The object can't be closed because it's in the wrong state. –  Kaz Oct 17 '12 at 3:04

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

share|improve this answer

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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