Est-ce une violation du principe de substitution de Liskov?

133

Supposons que nous ayons une liste d'entités de tâche et un ProjectTasksous-type. Les tâches peuvent être fermées à tout moment, à l'exception des tâches ProjectTasksqui ne peuvent pas être fermées une fois qu'elles ont le statut Lancé. L’interface utilisateur doit s’assurer que l’option de fermeture d’un espace ouvert ProjectTaskn’est jamais disponible, mais certaines garanties sont présentes dans le domaine:

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

Désormais, lors de l'appel Close()d'une tâche, il est possible que l'appel échoue s'il s'agit d'un ProjectTaskétat démarré, alors que ce ne serait pas le cas s'il s'agissait d'une tâche de base. Mais ce sont les exigences commerciales. Cela devrait échouer. Cela peut-il être considéré comme une violation du principe de substitution de Liskov ?

Paul T Davies
la source
14
Parfait pour un exemple de violation de substitution de liskov. Ne pas utiliser l'héritage ici, et tout ira bien.
Jimmy Hoffa
8
Vous pouvez modifier à: public Status Status { get; private set; }; sinon, la Close()méthode peut être contournée.
Job
5
Peut-être que c'est juste cet exemple, mais je ne vois aucun avantage matériel à se conformer au LSP. Pour moi, la solution proposée dans la question est plus claire, plus facile à comprendre et à gérer que celle qui est conforme à la norme LSP.
Ben Lee
2
@BenLee Ce n'est pas plus facile à maintenir. Cela n’a l’air que de cette façon parce que vous voyez cela isolément. Lorsque le système est volumineux, il est important de s’assurer que les sous-types de Taskn’introduisent pas d’incompatibilités bizarres dans le code polymorphe Task. Le LSP n'est pas un caprice, mais a été introduit précisément pour faciliter la maintenance dans les grands systèmes.
Andres F.
8
@ BenLee Imaginez que vous avez un TaskCloserprocessus qui closesAllTasks(tasks). Ce processus ne tente évidemment pas d'attraper les exceptions; après tout, cela ne fait pas partie du contrat explicite de Task.Close(). Maintenant, vous introduisez ProjectTasket, tout à coup, TaskClosercommence à lancer des exceptions (éventuellement non gérées). Ceci est une grosse affaire!
Andres F.

Réponses:

174

Oui, c'est une violation du LSP. Le principe de substitution de Liskov exige que

  • Les conditions préalables ne peuvent pas être renforcées dans un sous-type.
  • Les post-conditions ne peuvent pas être affaiblies dans un sous-type.
  • Les invariants du supertype doivent être conservés dans un sous-type.
  • Contrainte d'historique (la "règle d'historique"). Les objets sont considérés comme modifiables uniquement par leurs méthodes (encapsulation). Comme les sous-types peuvent introduire des méthodes qui ne sont pas présentes dans le supertype, leur introduction peut permettre des changements d'état dans le sous-type qui ne sont pas autorisés dans le supertype. La contrainte de l'histoire l'interdit.

Votre exemple casse la première condition en renforçant une condition préalable à l'appel de la Close()méthode.

Vous pouvez résoudre ce problème en amenant la condition préalable renforcée au niveau supérieur de la hiérarchie d'héritage:

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

En stipulant qu'un appel de Close()n'est valide que dans l'état où les CanClose()retours truevous font que la condition préalable s'applique à la Taskainsi qu'à la ProjectTask, réparant la violation LSP:

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();
    }
}
dasblinkenlight
la source
17
Je n'aime pas la duplication de ce chèque. Je préférerais une exception en lançant dans Task.Close et supprimer virtuel de Close.
Euphoric
4
@Euphoric C'est vrai, Closefaire vérifier le niveau supérieur et ajouter un protégé DoCloseserait une alternative valable. Cependant, je voulais rester aussi proche que possible de l'exemple du PO; l’améliorer est une question distincte.
dasblinkenlight
5
@Euphoric: Mais maintenant, il n'y a aucun moyen de répondre à la question: "Cette tâche peut-elle être fermée?" sans essayer de le fermer. Cela oblige inutilement à utiliser des exceptions pour le contrôle de flux. J'admettrai cependant que ce genre de chose peut être poussé trop loin. Pris trop loin, ce type de solution peut finir par créer un gâchis d’entreprise. Quoi qu'il en soit, la question du PO me semble être davantage une question de principes. Une réponse de la tour d'ivoire est donc tout à fait appropriée. +1
Brian
30
@ Brian Le CanClose est toujours là. Il peut toujours être appelé pour vérifier si la tâche peut être fermée. L'enregistrement dans Close devrait également appeler cela.
Euphoric
5
@Euphoric: Ah, j'ai mal compris. Vous avez raison, cela en fait une solution beaucoup plus propre.
Brian
82

Oui. Cela viole le LSP.

Ma suggestion est d'ajouter une CanCloseméthode / propriété à la tâche de base afin que toute tâche puisse indiquer si une tâche dans cet état peut être fermée. Cela peut aussi fournir une raison. Et retirez le virtuel de Close.

Basé sur mon commentaire:

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);
    }
}
Euphorique
la source
3
Merci pour cela, vous avez poussé plus loin dans l’exemple de dasblinkenlight, mais j’ai aimé son explication et sa justification. Désolé je ne peux pas accepter 2 réponses!
Paul T Davies
Je suis intéressé de savoir pourquoi la signature est publique virtuelle bool CanClose (out String, motif) - en utilisant out, êtes-vous simplement prêt pour l'avenir? Ou y a-t-il quelque chose de plus subtil qui me manque?
Reacher Gilt
3
@ReacherGilt Je pense que vous devriez vérifier ce que font / réf et lire mon code à nouveau. Tu es confus. Simplement "Si la tâche ne peut pas se fermer, je veux savoir pourquoi."
Euphoric
2
out n'est pas disponible dans toutes les langues, renvoyant un tuple (ou un simple objet encapsulant la raison et le booléen, il serait plus facile à transporter entre les langages OO, même au prix de perdre la facilité d'avoir directement un bool. Cela dit, pour les langages qui DO soutien, rien à redire à cette réponse
Newtopian
1
Et est-il acceptable de renforcer les conditions préalables pour la propriété CanClose? C'est-à-dire ajouter la condition?
John V
24

Le principe de substitution de Liskov stipule qu'une classe de base doit pouvoir être remplacée par l'une de ses sous-classes sans modifier les propriétés souhaitables du programme. Etant donné que ne ProjectTasksoulève une exception à la fermeture, un programme devrait être modifié pour tenir compte de cela, il devrait ProjectTaskêtre utilisé à la place de Task. C'est donc une violation.

Mais si vous modifiez en Taskindiquant dans sa signature qu'il peut déclencher une exception à la fermeture, vous ne violeriez pas le principe.

Tulains Córdova
la source
J'utilise c # ce qui, à mon avis, n'a pas cette possibilité, mais je sais que Java en a.
Paul T Davies
2
@PaulTDavies Vous pouvez décorer une méthode avec les exceptions qu'elle génère, msdn.microsoft.com/en-us/library/5ast78ax.aspx . Vous le remarquerez lorsque vous survolerez une méthode à partir de la bibliothèque de classes de base, vous obtiendrez une liste d'exceptions. Il n'est pas appliqué, mais il en avertit néanmoins l'appelant.
Despertar
18

Une violation de LSP nécessite trois parties. Le type T, le sous-type S et le programme P qui utilise T mais se voit attribuer une instance de S.

Votre question a fourni T (tâche) et S (tâche de projet), mais pas P. Donc, votre question est incomplète et la réponse est qualifiée: s'il existe un P qui ne s'attend pas à une exception, alors, pour ce P, vous avez un LSP violation. Si chaque P s'attend à une exception, il n'y a pas de violation LSP.

Cependant, vous n'avez une SRP violation. Le fait que l'état d'une tâche puisse être modifié et la politique selon laquelle certaines tâches dans certains États ne doivent pas être modifiées pour devenir d'autres États sont deux responsabilités très différentes.

  • Responsabilité 1: Représenter une tâche.
  • Responsabilité 2: Mettre en œuvre les stratégies qui modifient l'état des tâches.

Ces deux responsabilités changent pour des raisons différentes et doivent donc être réparties dans des classes distinctes. Les tâches doivent gérer le fait d'être une tâche et les données associées à une tâche. TaskStatePolicy doit gérer la manière dont les tâches passent d'un état à l'autre dans une application donnée.

Robert Martin
la source
2
Les responsabilités dépendent fortement du domaine et (dans cet exemple) de la complexité des états de tâche et de leurs changeurs. Dans ce cas, rien n'indique une telle chose, il n'y a donc aucun problème avec SRP. En ce qui concerne la violation LSP, je pense que nous avons tous supposé que l'appelant ne s'attendait pas à une exception et que l'application devrait afficher un message raisonnable au lieu d'entrer dans un état erroné.
Euphoric
Unca 'Bob répond? "Nous ne sommes pas dignes! Nous ne sommes pas dignes!". Quoi qu'il en soit ... Si chaque P s'attend à une exception, il n'y a pas de violation LSP. MAIS si nous stipulons qu'une instance T ne peut pas lancer un OpenTaskException(indice, indice) et que chaque P s'attend à une exception, que dit-il du code à l'interface, et non de la mise en œuvre? De quoi est-ce que je parle? Je ne sais pas. Je suis juste convaincu que je commente une réponse de Unca 'Bob.
radarbob
3
Vous avez raison de dire que prouver une violation LSP nécessite trois objets. Cependant, la violation de LSP existe s'il y a TOUT programme P qui était correct en l'absence de S mais échoue avec l'ajout de S.
kevin cline
16

Cela peut ou peut ne pas être une violation du LSP.

Sérieusement. Écoutez-moi.

Si vous suivez le LSP, les objets de type ProjectTaskdoivent se comporter comme des objets de type Taskdoivent se comporter.

Le problème avec votre code est que vous n'avez pas documenté la manière dont les objets de type Taskdoivent se comporter. Vous avez écrit du code, mais pas de contrat. Je vais ajouter un contrat pour Task.Close. Selon le contrat que j'ajoute, le code ProjectTask.Closecorrespondant ou non au LSP.

Compte tenu du contrat suivant pour Task.Close, le code de ProjectTask.Close ne suit pas le 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;
     }

Étant donné le contrat suivant pour Task.Close, le code pour ProjectTask.Close ne suivre le 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;
     }

Les méthodes pouvant être remplacées doivent être documentées de deux manières:

  • Le "Comportement" décrit ce sur quoi un client peut compter, sachant que l’objet destinataire est un Task, mais ne sait pas de quelle classe il est une instance directe. Il indique également aux concepteurs des sous-classes quelles substitutions sont raisonnables et lesquelles ne le sont pas.

  • Le "comportement par défaut" décrit ce à quoi un client peut se fier et sait que l'objet destinataire est une instance directe de Task(ce que vous obtenez si vous l'utilisez new Task(). Il indique également aux concepteurs des sous-classes quel comportement sera hérité s'ils ne le font pas. remplacer la méthode.

Maintenant, les relations suivantes devraient tenir:

  • Si S est un sous-type de T, le comportement documenté de S doit préciser le comportement documenté de T.
  • Si S est un sous-type de T (ou est égal à) T, le comportement du code de S doit affiner le comportement documenté de T.
  • Si S est un sous-type de T (ou est égal à) T, le comportement par défaut de S devrait préciser le comportement documenté de T.
  • Le comportement réel du code pour une classe doit affiner son comportement par défaut documenté.
Theodore Norvell
la source
@ user61852 a soulevé le point que vous pouvez indiquer dans la signature de la méthode qu'elle peut déclencher une exception, et que vous procédez ainsi simplement (quelque chose qui n'a pas de code d'effet réel), vous ne rompez plus le protocole LSP.
Paul T Davies
@PaulTDavies Vous avez raison. Mais dans la plupart des langues, la signature n'est pas un bon moyen de déclarer qu'une routine peut générer une exception. Par exemple, dans l'OP (en C #, je pense), la deuxième implémentation de " Closethrow". Donc, la signature déclare qu'une exception peut être levée - elle ne dit pas que ce ne sera pas le cas. Java fait un meilleur travail à cet égard. Même dans ce cas, si vous déclarez qu'une méthode peut déclarer une exception, vous devez documenter les circonstances dans lesquelles elle peut (ou le fera). Je soutiens donc que pour être sûr que le LSP soit violé, nous avons besoin de documentation au-delà de la signature.
Theodore Norvell
4
Beaucoup de réponses ici semblent ignorer complètement le fait que vous ne pouvez pas savoir si un contrat est validé si vous ne le connaissez pas. Merci pour cette réponse.
gnasher729
Bonne réponse, mais les autres réponses sont bonnes aussi. Ils en déduisent que la classe de base ne jette pas d'exception car il n'y a rien dans cette classe qui montre des signes de cela. Le programme, qui utilise la classe de base, ne doit donc pas se préparer à des exceptions.
inf3rno
Vous avez raison de dire que la liste des exceptions devrait être documentée quelque part. Je pense que le meilleur endroit est dans le code. Il y a une question connexe ici: stackoverflow.com/questions/16700130/... Mais vous pouvez le faire sans annotations, etc ... aussi, il suffit d' écrire quelque chose comme if (false) throw new Exception("cannot start")à la classe de base. Le compilateur va le supprimer, et le code contient toujours ce qui est nécessaire. Btw. nous avons toujours une violation LSP avec ces solutions de contournement, car la condition préalable est encore renforcée ...
inf3rno 10/10
6

Ce n'est pas une violation du principe de substitution de Liskov.

Le principe de substitution de Liskov dit:

Que q (x) soit une propriété démontrable sur les objets x de type T . Soit S être un sous - type de T . Le type S viole le principe de substitution de Liskov s'il existe un objet y de type S , tel que q (y) n'est pas prouvable.

La raison pour laquelle votre implémentation du sous-type n'est pas une violation du principe de substitution de Liskov est très simple: rien ne peut être prouvé sur ce qui Task::Close()se passe réellement. Bien sûr, ProjectTask::Close()jette une exception quand Status == Status.Started, mais pourrait le faire Status = Status.Closeddans Task::Close().

Oswald
la source
4

Oui, c'est une violation.

Je suggérerais que vous ayez votre hiérarchie à l'envers. Si tout Taskn'est pas fermable, alors close()n'appartient pas à Task. Peut-être que vous voulez une interface, CloseableTaskque tous les non- ProjectTaskspeuvent implémenter.

Tom G
la source
3
Chaque tâche peut être fermée, mais pas dans toutes les circonstances.
Paul T Davies
Cette approche me semble risquée, car les gens peuvent écrire du code en s'attendant à ce que toutes les tâches implémentent ClosableTask, bien qu'elles modélisent avec précision le problème. Je suis déchiré entre cette approche et une machine à états parce que je déteste les machines à états.
Jimmy Hoffa
Si Taskelle-même n’implémente pas, CloseableTaskils font un casting peu sûr, même quelque part Close().
Tom G
@TomG c'est ce dont j'ai peur
Jimmy Hoffa
1
Il existe déjà une machine à états. L'objet ne peut pas être fermé car il est dans le mauvais état.
Kaz
3

En plus d'être un problème lié au fournisseur de services linguistiques, il semble qu'il utilise des exceptions pour contrôler le flux de programmes (je dois supposer que vous capturez cette exception triviale quelque part et effectuez un flux personnalisé plutôt que de le laisser bloquer votre application).

Il semble que ce soit un bon endroit pour implémenter le modèle d'état pour TaskState et laisser les objets d'état gérer les transitions valides.

Ed Hastings
la source
1

Il me manque ici un élément important lié aux prestataires de services linguistiques et à la conception par contrat - dans les conditions préalables, c’est à l’appelant qui a la responsabilité de s’assurer que les conditions préalables sont remplies. Le code appelé, en théorie DbC, ne devrait pas vérifier la condition préalable. Le contrat doit spécifier quand une tâche peut être fermée (par exemple, CanClose renvoie True), puis le code appelant doit garantir que la condition préalable est remplie avant d'appeler Close ().

Ezoela Vacca
la source
Le contrat doit spécifier le comportement dont l'entreprise a besoin. Dans ce cas, Close () lève une exception lorsqu’il est appelé sur un serveur lancé ProjectTask. C'est une post-condition (cela dit ce qui se passe après l'appel de la méthode) et sa réalisation incombe au code appelé.
Goyo
@Goyo Oui, mais comme d'autres l'ont dit, l'exception est levée dans le sous-type, ce qui renforce la condition préalable et viole ainsi le contrat (implicite) selon lequel l'appel de Close () ferme simplement la tâche.
Ezoela Vacca
Quelle condition préalable? Je n'en vois pas.
Goyo
@Goyo Vérifiez la réponse acceptée, par exemple :) Dans la classe de base, Close n'a aucune condition préalable, il est appelé et la tâche est fermée. Chez l'enfant, cependant, il existe une condition préalable pour que le statut ne soit pas démarré. Comme d'autres l'ont souligné, il s'agit de critères plus stricts et le comportement n'est donc pas substituable.
Ezoela Vacca
Qu'à cela ne tienne, j'ai trouvé la condition préalable à la question. Mais alors il n’ya rien de mal (DbC-wise) avec les conditions préalables à la vérification de code appelées et à la levée d’exceptions lorsque celles-ci ne sont pas remplies. C'est ce qu'on appelle "la programmation défensive". En outre, s'il existe une post-condition indiquant ce qui se passe lorsque la condition préalable n'est pas remplie, dans ce cas, l'implémentation doit vérifier la condition préalable afin de s'assurer que la condition préalable est remplie.
Goyo
0

Oui, c'est une violation claire de LSP.

Certaines personnes soutiennent ici que rendre explicite dans la classe de base que les sous-classes peuvent renvoyer des exceptions rendrait cela acceptable, mais je ne pense pas que ce soit le cas. Peu importe ce que vous documentez dans la classe de base ou le niveau d'abstraction vers lequel vous déplacez le code, les conditions préalables seront toujours renforcées dans la sous-classe, car vous y ajoutez la partie "Impossible de fermer une tâche de projet commencée". Ce problème ne peut pas être résolu par une solution de contournement, il vous faut un modèle différent, qui ne viole pas le LSP (ou il faut relâcher la contrainte "les conditions préalables ne peuvent pas être renforcées").

Vous pouvez essayer le motif de décorateur si vous souhaitez éviter la violation LSP dans ce cas. Cela pourrait fonctionner, je ne sais pas.

inf3rno
la source