LSP violation or not

[Jeroen De Dauw]

There appears to be some disagreement on if the following is a LSP violation or not: https://gist.github.com/JeroenDeDauw/f5d3ce3e184f31f0b776

Two questions:

1. do you think this is a LSP violation?
2. do you think this is harmful? (and in case you do, what would be less harmful?)

[w3bj...@gmail.com]

1. Yes

2. Yes

What's the point of defining the interface in the first place ?

[Jeroen De Dauw]

Just came across something else that falls into this category though is easier to understand on its own.

http://pastebin.com/i5MTuh96

Implementations of this interface typically deal with a single derivative of Description. The interface is used by having a list of instances and doing a loop in the form

foreach ( $compilers as $compiler ) {
    if ( $compiler->canCompileDescription( $stuff ) ) return $compiler->compileDescription( $stuff );
}

> 1. Yes

> 2. Yes

Can you perhaps provide a little motivation?

[Dave Hunt]

Definitely an LSP violation. What is the purpose of the program? I can't suggest an alternative without knowing how these classes would be used.

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

[Jeroen De Dauw]

> I can't suggest an alternative without knowing how these classes would be used.

Did you look at the second example I posted? I figure it's easy enough to understand the pattern there. In general it goes as follows:

Scenario: you have a type (a value) with a non-fixed number of sub types, perhaps with the need for extensions to your system to be able to add their own. Your system needs to be able to do something with instances of this type that requires specific handling for specific subtypes. This might be some kind of transformation, displaying of data, persisting it, etc. It's not a good idea to handle whatever this task is into the type interface, since that'd be an SRP violation.

Solution: create a second type that handles the task, and has an interface in the form

* canHandle( Type )
* handle( Type )

Then your system can have a list of this second type and easily apply the task on the original type via a simple foreach as shown before. No SRP or OCP violation - extensions can happily add their own derivatives now.

(Does this pattern have a name?)

I can see how this is a LSP violation if you look purely at the type system. If you also look at the contract of the interface, I'm more inclined to say that this code runs into a limitation of the type system rather than an actual LSP violation. Though perhaps it's not that valuable to discuss semantics here. What I find more interesting is that the problems that arise from your classical LSP violation appear to be absent. Those problems being unexpected behavior and OCP violations to get around them. If I'm right about these problems being absent, then clearly this LSP violation is not harmful, suggesting one should distinguish between two different classes of LSP violations.

I'm very curious to hear about issues and alternatives.

[Karl Miller]

I've used this exact pattern when doing the (extended) FizzBuzz kata.

It somehow feels a little clunky, but I've never been able to pin down why. Your elucidation helps I think. I'll do the kata again and see if I think of anything.

Cheers
Karl

--

[w3bj...@gmail.com]

This second example is a lot better IMHO, because the boolean predicate changes everything.

[Jeroen De Dauw]

> This second example is a lot better IMHO, because the boolean predicate changes everything.

It encodes the restriction in the interface signature in a way. The reason for doing so is not to avoid the LSP violation though, it's to enable the loop. You can imagine adding an "bool acceptsId( Id $id )" method to the Identifiable interface in the first example. Why do that if you do not have a use for it though?

I'm still waiting for an explanation of why the first example is harmful. The LSP violation prevents you from doing things such as

* $kitten->setId( new PonyId( 'P42' ) );
* $animal->setId( new PonyId( 'P42' ) );

These things are wrong, so what is the harm in them not working? It's also clear from looking at the interface that you cannot do this. I know of a sizable system that has had this for the past 3 years, and AFAIK no one ever got confused by this or had to create hacks to deal with it.

[Łukasz Duda]

I don't see much common between Kitten and Pony. If Pony needs PonyId it shouldn't implement Identifiable in my opinion. I don't get it.

[w3bj...@gmail.com]

I think that there is a misunderstanding of what interfaces are.

If you want to use an interface properly, you need to typehint everything.

Like this :

interface Identifiable {
        public function getId();
        public function setId( Id $id );
}

That interface state that every object that implements it can accept an objet of type Id as id.

If your Kitten class doesn't, then it is a LSP violation.

Le dimanche 1 février 2015 17:47:53 UTC+1, Jeroen De Dauw a écrit :

[w3bj...@gmail.com]

I think that there is a misunderstanding of what interfaces are.

If you want to use an interface properly, you need to typehint everything.

Like this :

interface Identifiable {
        public function getId();
        public function setId( Id $id );
}

That interface states that every object that implements it can accept an objet of type Id as id.

[Juan Manuel Gimeno Illa]

But, IMHO, LSP's violations are respect what the clients of a classe expect the behaviour of their instances to be (independently of the subclass they belong to). 

In this case, what is the expected behaviour of setId? To succed or fail in a public and determined manner (throwing an exception). 

So clients who want to use Identifiable instances have to protect themselves against InvalidArgumentException. So client code which does not check if the exception has been thrown is wrong.

The behaviour of the subclasses is compatible with that, so any instance of the subclasses works well with correct code which uses the public behaviour of Identifiable.

So, IMHO, no LSP violation.

Juan Manuel

[w3bj...@gmail.com]

IMHO, Intent should be readable in code :

When there is no method "can_accept_id( $id )" then the code doesn't state properly the intent of the interface.

" Comment should be considered as failure "

[Juan Manuel Gimeno Illa]

You are right but then what is violated is "good naming" not LSP.

JM

[David Stibbe]

It is most definitely a violation of LSP. 

It violates the following:

• Preconditions cannot be strengthened in a subtype.

The fact that you suddenly reduce the accepted set of accepted Ids in a subclass like Pony, strengthens the precondition. Even worse, the precondition is different for each subclass... 

It is also impossible to switch a Pony with a Kitten without breaking stuff.

Perhaps you want the following:

<?php

interface Identifiable {

	/**
	 * @return Id
	 */
	public function getId();

}

class Kitten implements Identifiable {
        private $kittenId;

        public function getId(){
            return $kittenId;
        }

	/**
	 * @param KittenId $id
	 */
	public function __construct( $id ) {
		if ( !( $id instanceof KittenId ) ) {
			throw new InvalidArgumentException( 'Needs to be a KittenId' );
		}
                $kittenId = $id;

	}

}

class Pony implements Identifiable {
        private $ponyId;

        public function getId(){
            return $ponyId;
        }

	/**
	 * @param PonyId $id
	 */
	public function __construct( $id ) {
		if ( !( $id instanceof PonyId ) ) {
			throw new InvalidArgumentException( 'Needs to be a PonyId' );
		}
                $ponyId = $id;

	}

}

abstract class Id {
	public abstract function toString();
}

class KittenId extends Id {
	// Makes sure all Kitten ids start with a "k"
}

class PonyId extends Id {
	// Makes sure all pony ids start with a "p"
}

--

[Indrit Selimi]

From Wikipedia: LSP
"It states that, in a computer program, if S is a subtype of T, then objects of type T may be replaced with objects of type S (i.e., objects of type S may substitute objects of type T) without altering any of the desirable properties of that program (correctness, task performed, etc.). "

"It is a semantic rather than merely syntactic relation because it intends to guarantee semantic interoperability of types in a hierarchy"

a) without altering any of the desirable properties of that program (correctness, task performed, etc.)
b) it is a semantic rather than merely syntactic

What in the given example is breaking the "desirable properties" ? (i.e. which is the desirable property being braked? )

Moreover: "Let q(x) be a property provable about objects x of type T. Then q(y) should be provable for objects y of type S, where S is a subtype of T."

In this case which is the q(x) being "not provable" ?