Is this an LSP viiolation?

[Terence McGhee]

There's a post on stack exchange here. Most there seem to think that this is an LSP violation. Personally, I don't think it is a violation because the base could also throw an exception (under some other circumstance like maybe Status === Status.Deleted or something).

However, I do agree that the accepted answer is better than the original, but not because it's a LSP violation. 

Thoughts?

[Andreas Altendorfer]

I agree with you, Terence. I don't see a LSP violation here. If you expect an exception you can use both Task and ProjectTask the same way.

But here's the big smell. As mentioned in comments, you should never use exceptions for flow-control. 

[Terence McGhee]

Thanks for the feedback!

Maybe I misunderstood the technique he was advocating, but I think in ep 5, UB suggests that exceptions are the best way to handle flow control for error conditions. 

[Andreas Altendorfer]

Sure, for error conditions I agree. But I think, trying to close a ProjectTask which is started isn't an error –of the system.

I would see it as part of the business logic and it should be handled before the close-action get's triggered. It should display a message to the user, like "you can't close a running task" but not throw an exception which may end up in system message the user will not understand.

Using a callback like can_close? would be much better here.

The system can't close (because of a disk-error or whatever) is an exceptional condition. User is not allowed to close is not.

best

- Andreas

[Ben Biddington]

So what about the fact that a concretion has polluted an abstraction?

Task now has CanClose which only makes sense for a derived type.

What other options are there for representing that behaviour? Could do nothing, but that might violate the expectation that a task has status closed after closing.

Ben

--
The only way to go fast is to go well.
---
You received this message because you are subscribed to the Google Groups "Clean Code Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to clean-code-discu...@googlegroups.com.
To post to this group, send email to clean-code...@googlegroups.com.
Visit this group at http://groups.google.com/group/clean-code-discussion.

[Terence McGhee]

Hello Ben.

As I said in my first post in this topic, another reason that the base type might need CanClose() is maybe Status == Status.Deleted or Status == Status.Closed.

@Andreas, ahh, I understand what you mean now about exceptions! Thanks again for the feedback!

[Caio Fernando Bertoldi Paes de Andrade]

Andreas

Using a callback like can_close? would be much better here.

I see this as a smell.

You defer to the caller the responsibility of validating the input, which I think is a bit too much knowledge.

You have to always call can_close? before trying to close, which smells like duplicated can_close?s in different places.

You introduce a sort of time coupling, having to always check can_close? before calling close, otherwise the business rule won’t apply.

I believe exceptions were made for this purpose and I find them more readable and clean.

Caio

[Terence McGhee]

I think that the CanClose() method would be implemented in the base, therefore no clients (or derived classes) need to concern themselves with making sure it's being called.

[Andreas Altendorfer]

Caio,

Though, I half agree with you. It depends on parts of the code I don't now. 

In general, I had very bad experiences with exceptions used in business-logic-flow.

If the ProjectTask throws an exception when save() is called while running, that's completely ok.

But I guess that's only half of the story and the application should not use this exception to, let's say 'check' the status of a ProjectTask.

Suggest you have a collection of tasks, some of them are ProjectTasks, some are others.

When you tell the collection to saveAll() you may receive the exception at a point where you deal with the collection and not a single task.

So, which tasks in the collection are now saved and which aren't?

Next step will be to catch the exception within the loop in saveAll() and while writing this code, this scaring question will come up, "where else will I have to do the same?"

As I mentioned above, without knowing the code I can't say if I would implement a canClose?-method or not. Usually I prefere not using exceptions for flow-control of not real exceptional circumstances.

best

- Andreas

You received this message because you are subscribed to a topic in the Google Groups "Clean Code Discussion" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clean-code-discussion/_GC5TwQUY84/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clean-code-discu...@googlegroups.com.

[Caio Fernando Bertoldi Paes de Andrade]

Andreas,

Suggest you have a collection of tasks, some of them are ProjectTasks, some are others.

When you tell the collection to saveAll() you may receive the exception at a point where you deal with the collection and not a single task.

So, which tasks in the collection are now saved and which aren't?

Next step will be to catch the exception within the loop in saveAll() […]

To cope with individual failures in a collection you would have to implement a canClose check inside the loop anyway.

An advantage of using Exceptions is that you can decouple the caller from its callee internal implementation. The validation is the same for all callers, but how to handle the exception can differ from caller to caller. A caller can catch a general Exception and show the informative error message in the Exception object to the user, without having to know what failed.

[…] while writing this code, this scaring question will come up, "where else will I have to do the same?"

If you have lots of different requirements and validations to do, which is quite likely in a real-world application, I would ask that very same question when I see a caller validating the inner state of its callee via canClose alike methods.

Cheers,
Caio

[Per Lundholm]

The problem with the "canClose" method is that the state may change between asking canClose and calling Close.

An exception is, as you say, to prefer with the added comment that it should be part of the API - how would the client else know that there is a possible exception? Now, lots of people hate checked exceptions and unchecked gets neglected, so you lose anyway you do it. :)

I'd prefer to change the design to "tell-don't ask", if possible. If I send the "close"-message to an object and that object does not allow closing, it just ignores it. 

That may not be an option as the example seem to say that if some client sends "close", it is considered an error and that's a business requirement. Ah, well.

Regards,

  Per

...

http://blog.crisp.se/perlundholm

2013/8/31 Caio Fernando Bertoldi Paes de Andrade <caio...@gmail.com>

--
Per Lundholm, Crisp AB
http://blog.crisp.se/perlundholm

[Terence McGhee]

If CanClose is implemented as such, then clients don't have to deal with making sure it's called, and it does honor tell-don't-ask, from the client's point of view, and it ensures that all variants behave in the same way.

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

    protected virtual bool CanClose() {
        return true;
    }

    public void Close() {
        if (CanClose() == false)
            throw new Exception(reason);

        Status = Status.Closed;
    }
}

[Uncle Bob]

I answered it this way:

<hr/>

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.  

[Terence McGhee]

@UncleBob

Wow that's an amazingly complete answer. It takes three parts to decide the LSP violation! HA! That's awesome! Now that you've mentioned it, it's so overwhelmingly clear.

But... spotting the SRP violation. That was real skill. Even after reading your explanation of why, it's still taking me time and effort to grasp it. Now I've been kicking myself and asking why didn't I notice it myself. 

The books, the videos, and this discussion board are a vital part of my continuing education in software craftsmanship. These concepts sometime knock me out for days, but I'm better because of it (when I finally come to; and wrap my mind around them).