Handelt es sich um eine Verletzung des Liskov-Substitutionsprinzips?

132

Angenommen, wir haben eine Liste von Aufgabenentitäten und einen ProjectTaskUntertyp. Aufgaben können jederzeit geschlossen werden, es sei denn ProjectTasks, sie können nicht geschlossen werden, sobald sie den Status Gestartet haben. Die Benutzeroberfläche sollte sicherstellen, dass die Option zum Schließen eines gestarteten ProjectTaskObjekts niemals verfügbar ist. In der Domäne sind jedoch einige Sicherheitsvorkehrungen vorhanden:

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

Wenn Sie nun Close()eine Aufgabe aufrufen , besteht die Möglichkeit, dass der Anruf fehlschlägt, wenn er ProjectTaskden Status "Gestartet" hat, während dies bei einer Basisaufgabe nicht der Fall wäre. Dies sind jedoch die geschäftlichen Anforderungen. Es sollte scheitern. Kann dies als Verstoß gegen das Liskov-Substitutionsprinzip angesehen werden ?

Paul T Davies
quelle
14
Perfekt für ein T-Beispiel für die Verletzung der Liskov-Substitution. Verwenden Sie hier keine Vererbung, und alles wird gut.
Jimmy Hoffa
8
Vielleicht möchten Sie es ändern in public Status Status { get; private set; }:; Andernfalls kann die Close()Methode umgangen werden.
Job
5
Vielleicht ist es nur dieses Beispiel, aber ich sehe keinen wesentlichen Vorteil darin, den LSP einzuhalten. Für mich ist diese Lösung in der Frage klarer, verständlicher und wartungsfreundlicher als die, die mit LSP übereinstimmt.
Ben Lee
2
@BenLee Es ist nicht einfacher zu pflegen. Es sieht nur so aus, weil Sie dies isoliert sehen. Wenn das System groß ist, ist es eine große Sache sicherzustellen, dass Subtypen von Taskkeine bizarren Inkompatibilitäten in polymorphen Code einführen, von denen nur bekannt Taskist. LSP ist keine Laune, sondern wurde speziell eingeführt, um die Wartbarkeit in großen Systemen zu verbessern.
Andres F.
8
@BenLee Stellen Sie sich vor, Sie haben einen TaskCloserProzess, der closesAllTasks(tasks). Dieser Prozess versucht offensichtlich nicht, Ausnahmen zu erfassen. es ist schließlich nicht Bestandteil des ausdrücklichen Vertrages von Task.Close(). Jetzt stellst du vor ProjectTaskund plötzlich TaskCloserbeginnt dein Auslösen von (möglicherweise unbehandelten) Ausnahmen. Das ist eine große Sache!
Andres F.

Antworten:

173

Ja, es ist eine Verletzung des LSP. Liskov Substitutionsprinzip erfordert , dass

  • Voraussetzungen können in einem Subtyp nicht gestärkt werden.
  • Nachbedingungen können in einem Subtyp nicht abgeschwächt werden.
  • Invarianten des Supertyps müssen in einem Subtyp erhalten bleiben.
  • Verlaufsbeschränkung (die "Verlaufsregel"). Objekte gelten nur durch ihre Methoden als veränderbar (Kapselung). Da Subtypen Methoden einführen können, die im Supertyp nicht vorhanden sind, kann die Einführung dieser Methoden Zustandsänderungen im Subtyp ermöglichen, die im Supertyp nicht zulässig sind. Die Geschichtsbedingung verbietet dies.

Ihr Beispiel unterbricht die erste Anforderung, indem es eine Voraussetzung für den Aufruf der Close()Methode verschärft .

Sie können das Problem beheben, indem Sie die verstärkte Vorbedingung auf die oberste Ebene der Vererbungshierarchie bringen:

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

Indem Sie festlegen, dass ein Aufruf von Close()nur in dem Zustand gültig ist, in dem die CanClose()Rücksendung erfolgt true, stellen Sie sicher , dass die Vorbedingung sowohl für die, Taskals auch für die ProjectTask, gilt.

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
quelle
17
Ich mag keine Vervielfältigung dieses Schecks. Ich würde es vorziehen, eine Ausnahme in Task.Close zu werfen und virtual aus Close zu entfernen.
Euphoric
4
@Euphoric Das ist wahr, wenn die oberste Ebene Closedie Überprüfung durchführt und ein geschütztes Element hinzugefügt wird, ist dies DoCloseeine gültige Alternative. Ich wollte jedoch dem Beispiel des OP so nahe wie möglich kommen. es zu verbessern ist eine separate Frage.
dasblinkenlight
5
@Euphoric: Aber jetzt gibt es keine Möglichkeit, die Frage zu beantworten: "Kann diese Aufgabe abgeschlossen werden?" ohne zu versuchen, es zu schließen. Dies erzwingt unnötigerweise die Verwendung von Ausnahmen für die Flusssteuerung. Ich gebe jedoch zu, dass so etwas zu weit gehen kann. Zu weit gegangen, kann diese Art der Lösung zu einem unternehmerischen Durcheinander führen. Ungeachtet dessen scheint mir die Frage des OP mehr nach Prinzipien zu sein, daher ist eine Antwort auf einen Elfenbeinturm sehr angemessen. +1
Brian
30
@Brian Der CanClose ist noch da. Es kann weiterhin aufgerufen werden, um zu überprüfen, ob die Aufgabe geschlossen werden kann. Das Einchecken in Schließen sollte dies auch aufrufen.
Euphoric
5
@Euphoric: Ah, ich habe es falsch verstanden. Sie haben Recht, das ergibt eine viel sauberere Lösung.
Brian
82

Ja. Dies verletzt LSP.

Mein Vorschlag ist, der Basisaufgabe eine CanCloseMethode / Eigenschaft hinzuzufügen , damit jede Aufgabe erkennen kann, ob die Aufgabe in diesem Zustand geschlossen werden kann. Es kann auch einen Grund dafür liefern. Und entfernen Sie die virtuelle aus Close.

Basierend auf meinem Kommentar:

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);
    }
}
Euphorisch
quelle
3
Vielen Dank dafür, dass Sie das Beispiel von dasblinkenlight eine Stufe weiter gebracht haben, aber mir hat seine Erklärung und Begründung gefallen. Entschuldigung, ich kann 2 Antworten nicht akzeptieren!
Paul T Davies
Ich bin interessiert zu wissen, warum die Signatur öffentlich ist. Virtual Bool CanClose (out String reason) - mit out sind Sie nur zukunftssicher? Oder fehlt mir etwas Feineres?
Reacher Gilt
3
@ReacherGilt Ich denke, Sie sollten überprüfen, was aus / ref tun und meinen Code noch einmal lesen. Du bist verwirrt. Einfach "Wenn die Aufgabe nicht abgeschlossen werden kann, möchte ich wissen, warum."
Euphoric
2
out ist nicht in allen Sprachen verfügbar. Wenn Sie ein Tupel zurückgeben (oder ein einfaches Objekt, das den Grund und den Booleschen Wert einkapselt, wäre es besser für OO-Sprachen übertragbar, wenn auch auf Kosten des Verlusts der Einfachheit, direkt einen Bool zu haben Unterstützen Sie, nichts falsch mit dieser Antwort
Newtopian
1
Und ist es in Ordnung, die Voraussetzungen für die CanClose-Eigenschaft zu verbessern? Dh die Bedingung hinzufügen?
John V
24

Das Liskov-Substitutionsprinzip besagt, dass eine Basisklasse durch eine seiner Unterklassen ersetzt werden kann, ohne dass die gewünschten Eigenschaften des Programms geändert werden. Da ProjectTaskbeim Schließen nur eine Ausnahme ausgelöst wird, müsste ein Programm geändert werden, um dem Rechnung zu tragen, ProjectTaskdas anstelle von verwendet werden sollte Task. Es ist also eine Verletzung.

Aber wenn Sie ändern Taskin seiner Unterschrift die besagt , dass es möglicherweise eine Ausnahme auslösen , wenn sie geschlossen, dann werden Sie sich nicht gegen den Grundsatz verstoßen.

Tulains Córdova
quelle
Ich benutze c #, von dem ich glaube, dass es diese Möglichkeit nicht gibt, aber ich weiß, dass Java dies tut.
Paul T Davies
2
@PaulTDavies Sie können eine Methode mit den von ihr ausgelösten Ausnahmen dekorieren: msdn.microsoft.com/en-us/library/5ast78ax.aspx . Wenn Sie den Mauszeiger über eine Methode aus der Basisklassenbibliothek bewegen, wird eine Liste mit Ausnahmen angezeigt. Es wird nicht erzwungen, macht den Anrufer jedoch darauf aufmerksam.
Despertar
18

Eine LSP-Verletzung erfordert drei Parteien. Der Typ T, der Subtyp S und das Programm P, das T verwendet, aber eine Instanz von S erhält.

Ihre Frage enthält T (Aufgabe) und S (Projektaufgabe), nicht jedoch P. Ihre Frage ist also unvollständig und die Antwort ist qualifiziert: Wenn ein P existiert, das keine Ausnahme erwartet, haben Sie für dieses P einen LSP Verletzung. Wenn jedes P eine Ausnahme erwartet, liegt keine LSP-Verletzung vor.

Aber Sie tun einen haben SRP Verletzung. Die Tatsache , dass der Zustand einer Aufgabe geändert werden kann, und die Politik , dass bestimmte Aufgaben in bestimmten Staaten sollen nicht auf andere Staaten, sind zwei sehr unterschiedliche Zuständigkeiten geändert werden.

  • Verantwortung 1: Eine Aufgabe darstellen.
  • Verantwortung 2: Implementieren Sie die Richtlinien, die den Status von Aufgaben ändern.

Diese beiden Verantwortlichkeiten ändern sich aus unterschiedlichen Gründen und sollten daher in getrennten Klassen sein. Aufgaben sollten die Tatsache behandeln, dass es sich um eine Aufgabe handelt, und die Daten, die mit einer Aufgabe verknüpft sind. TaskStatePolicy sollte den Übergang von Aufgaben von Status zu Status in einer bestimmten Anwendung behandeln.

Robert Martin
quelle
2
Die Zuständigkeiten hängen stark von der Domäne und (in diesem Beispiel) davon ab, wie komplex die Aufgabenzustände und ihre Wechsler sind. In diesem Fall gibt es keinen Hinweis darauf, sodass SRP kein Problem darstellt. Was die LSP-Verletzung betrifft, sind wir alle davon ausgegangen, dass der Anrufer keine Ausnahme erwartet und die Anwendung eine angemessene Meldung anzeigen sollte, anstatt in einen fehlerhaften Zustand zu geraten.
Euphoric
Unca 'Bob antwortet? "Wir sind es nicht wert! Wir sind es nicht wert!" Wie auch immer ... Wenn jedes P eine Ausnahme erwartet, liegt keine LSP-Verletzung vor. ABER wenn wir festlegen, dass eine T-Instanz keinen OpenTaskException(Hinweis, Hinweis) auslösen kann und jedes P eine Ausnahme erwartet, was sagt das dann über den Code zur Schnittstelle aus, nicht über die Implementierung? Worüber rede ich? Ich weiß es nicht. Ich bin nur begeistert, dass ich eine Antwort von Unca 'Bob kommentiere.
Radarbob
3
Sie haben Recht, dass für den Nachweis einer LSP-Verletzung drei Objekte erforderlich sind. Es besteht jedoch die LSP Verletzung , wenn es ein Programm P ist , die in Abwesenheit von S korrekt war , aber nicht mit dem Zusatz von S.
kevin Cline
16

Dies kann eine Verletzung des LSP sein oder nicht .

Ernsthaft. Lass mich ausreden.

Wenn Sie dem LSP folgen, ProjectTaskmüssen sich Objekte vom Typ Taskso verhalten, wie es von Objekten vom Typ erwartet wird.

Das Problem mit Ihrem Code ist, dass Sie nicht dokumentiert haben, wie sich Objekte des Typs Taskvoraussichtlich verhalten. Sie haben Code geschrieben, aber keine Verträge. Ich werde einen Vertrag für hinzufügen Task.Close. Abhängig vom Vertrag, den ich hinzufüge, ProjectTask.Closefolgt der Code für entweder dem LSP oder nicht.

In Anbetracht des folgenden Vertrags für Task.Close folgt der Code für ProjectTask.Close nicht dem 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;
     }

Angesichts der folgenden Auftrag für Task.Close, den Code für ProjectTask.Close sich die LSP wie folgt vor :

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

Methoden, die möglicherweise überschrieben werden, sollten auf zwei Arten dokumentiert werden:

  • Das "Verhalten" dokumentiert, worauf sich ein Client verlassen kann, der das Empfängerobjekt a kennt Task, aber nicht weiß, von welcher Klasse es eine direkte Instanz ist. Außerdem erfahren Designer von Unterklassen, welche Überschreibungen sinnvoll und welche nicht sinnvoll sind.

  • Das "Standardverhalten" dokumentiert, worauf sich ein Client verlassen kann, der weiß, dass das Empfängerobjekt eine direkte Instanz von ist Task(dh was Sie erhalten, wenn Sie es verwenden new Task(). Es teilt den Designern von Unterklassen auch mit, welches Verhalten vererbt wird, wenn dies nicht der Fall ist überschreiben Sie die Methode.

Nun sollten folgende Beziehungen gelten:

  • Wenn S ein Subtyp von T ist, sollte das dokumentierte Verhalten von S das dokumentierte Verhalten von T verfeinern.
  • Wenn S ein Subtyp von (oder gleich) T ist, sollte das Verhalten des S-Codes das dokumentierte Verhalten von T verfeinern.
  • Wenn S ein Subtyp von (oder gleich) T ist, sollte das Standardverhalten von S das dokumentierte Verhalten von T verfeinern.
  • Das tatsächliche Verhalten des Codes für eine Klasse sollte das dokumentierte Standardverhalten verfeinern.
Theodore Norvell
quelle
@ user61852 hat den Punkt angesprochen, dass Sie in der Signatur der Methode angeben können, dass sie eine Ausnahme auslösen kann, und indem Sie dies einfach tun (etwas, das keinen tatsächlichen Effektcode aufweist), brechen Sie LSP nicht mehr.
Paul T Davies
@PaulTDavies Du hast recht. In den meisten Sprachen ist die Signatur jedoch kein guter Weg, um zu deklarieren, dass eine Routine eine Ausnahme auslösen kann. Zum Beispiel im OP (in C #, denke ich) Closewirft die zweite Implementierung von . So erklärt die Unterschrift , dass eine Ausnahme kann ausgelöst werden - es sagt nicht , dass man nicht. Java macht diesbezüglich einen besseren Job. Wenn Sie jedoch deklarieren, dass eine Methode eine Ausnahme deklarieren könnte, sollten Sie die Umstände dokumentieren, unter denen dies möglich ist (oder sein wird). Um sicherzugehen, ob gegen LSP verstoßen wird, ist eine Dokumentation erforderlich, die über die Signatur hinausgeht.
Theodore Norvell
4
Viele Antworten hier scheinen die Tatsache völlig zu ignorieren, dass Sie nicht wissen können, ob ein Vertrag gültig ist, wenn Sie den Vertrag nicht kennen. Danke für diese Antwort.
gnasher729
Gute Antwort, aber die anderen Antworten sind auch gut. Sie schließen daraus, dass die Basisklasse keine Ausnahme auslöst, weil in dieser Klasse nichts vorhanden ist, was Anzeichen dafür aufweist. Daher sollte sich das Programm, das die Basisklasse verwendet, nicht auf Ausnahmen vorbereiten.
Inf3rno
Sie haben Recht, dass die Ausnahmeliste irgendwo dokumentiert werden sollte. Ich denke, der beste Platz ist im Code. Hier gibt es eine verwandte Frage: stackoverflow.com/questions/16700130/…. Sie können dies jedoch auch ohne Anmerkungen usw. tun. Schreiben Sie einfach so etwas wie if (false) throw new Exception("cannot start")an die Basisklasse. Der Compiler wird es entfernen, und der Code enthält immer noch, was benötigt wird. Btw. Wir haben immer noch eine LSP-Verletzung mit diesen Workarounds, weil die Vorbedingung immer noch gestärkt ist ...
Inf3rno 10.10.17
6

Es ist keine Verletzung des Liskov-Substitutionsprinzips.

Das Liskov-Substitutionsprinzip besagt:

Lassen Sie q (x) eine Eigenschaft beweisbar über Objekte sein x vom Typ T . Lassen S ein Subtyp von seiner T . Typ S verstößt gegen das Liskov-Substitutionsprinzip, wenn ein Objekt y vom Typ S existiert, so dass q (y) nicht nachweisbar ist.

Der Grund, warum Ihre Implementierung des Subtyps nicht gegen das Liskov-Substitutionsprinzip verstößt, ist ganz einfach: Es kann nicht bewiesen werden, was Task::Close()tatsächlich geschieht. Sicher, ProjectTask::Close()wirft eine Ausnahme , wenn Status == Status.Started, aber so könnte Status = Status.Closedin Task::Close().

Oswald
quelle
4

Ja, es ist eine Verletzung.

Ich würde vorschlagen, dass Sie Ihre Hierarchie rückwärts haben. Wenn nicht jeder Taskverschließbar ist, dann close()gehört das nicht rein Task. Vielleicht möchten Sie eine Schnittstelle, CloseableTaskdie alle nicht ProjectTasksimplementieren können.

Tom G
quelle
3
Jede Aufgabe ist abschließbar, aber nicht unter allen Umständen.
Paul T Davies
Dieser Ansatz scheint mir riskant zu sein, da Benutzer möglicherweise Code schreiben, der erwartet, dass alle Tasks ClosableTask implementieren, obwohl das Problem dadurch genau modelliert wird. Ich bin hin und her gerissen zwischen diesem Ansatz und einer Zustandsmaschine, weil ich Zustandsmaschinen hasse.
Jimmy Hoffa
Wenn Taskdies nicht selbst implementiert CloseableTaskwird, führen sie eine unsichere Umwandlung durch, um überhaupt aufzurufen Close().
Tom G
@ TomG das ist, wovor ich Angst habe
Jimmy Hoffa
1
Es gibt bereits eine Zustandsmaschine. Das Objekt kann nicht geschlossen werden, da es sich im falschen Zustand befindet.
Kaz
3

Es scheint nicht nur ein LSP-Problem zu sein, sondern es werden auch Ausnahmen verwendet, um den Programmfluss zu steuern.

Anscheinend ist dies ein guter Ort, um das Statusmuster für TaskState zu implementieren und die Statusobjekte die gültigen Übergänge verwalten zu lassen.

Ed Hastings
quelle
1

Mir fehlt hier eine wichtige Sache in Bezug auf LSP und Design by Contract - in den Voraussetzungen ist es der Anrufer, der dafür verantwortlich ist, sicherzustellen, dass die Voraussetzungen erfüllt sind. Der aufgerufene Code sollte in der DbC-Theorie die Vorbedingung nicht verifizieren. Der Vertrag sollte angeben, wann eine Aufgabe geschlossen werden kann (z. B. CanClose gibt True zurück), und der aufrufende Code sollte sicherstellen, dass die Vorbedingung erfüllt ist, bevor er Close () aufruft.

Ezoela Vacca
quelle
Im Vertrag sollte festgelegt werden, welches Verhalten das Unternehmen benötigt. In diesem Fall löst Close () eine Ausnahme aus, wenn es beim Start aufgerufen wird ProjectTask. Dies ist eine Nachbedingung (sie besagt, was nach dem Aufrufen der Methode geschieht), und die Erfüllung dieser Bedingung liegt in der Verantwortung des aufgerufenen Codes.
Goyo
@Goyo Ja, aber wie andere sagten, wird die Ausnahme in dem Subtyp ausgelöst, der die Vorbedingung stärkt und damit den (impliziten) Vertrag verletzt, dass der Aufruf von Close () die Aufgabe einfach schließt.
Ezoela Vacca
Welche Voraussetzung? Ich sehe keine.
Goyo
@Goyo Überprüfe die akzeptierte Antwort, zum Beispiel :) In der Basisklasse hat Close keine Vorbedingungen, sie wird aufgerufen und beendet die Aufgabe. Beim Kind besteht jedoch die Voraussetzung, dass der Status nicht gestartet wird. Wie andere betonten, handelt es sich um strengere Kriterien, und das Verhalten ist daher nicht ersetzbar.
Ezoela Vacca
Egal, ich habe die Voraussetzung in der Frage gefunden. Aber dann ist nichts falsch (DbC-weise), wenn der aufgerufene Code die Voraussetzungen überprüft und Ausnahmen auslöst, wenn sie nicht erfüllt werden. Es heißt "defensive Programmierung". Wenn darüber hinaus eine Nachbedingung vorliegt, die angibt, was passiert, wenn die Vorbedingung nicht erfüllt ist, wie in diesem Fall, muss die Implementierung die Vorbedingung überprüfen, um sicherzustellen, dass die Nachbedingung erfüllt ist.
Goyo
0

Ja, es ist eine eindeutige Verletzung von LSP.

Einige Leute argumentieren hier, dass es akzeptabel wäre, in der Basisklasse explizit zu machen, dass die Unterklassen Ausnahmen auslösen können, aber ich denke nicht, dass das wahr ist. Unabhängig davon, was Sie in der Basisklasse dokumentieren oder auf welche Abstraktionsebene Sie den Code verschieben, werden die Voraussetzungen in der Unterklasse weiter verbessert, da Sie den Teil "Gestartete Projektaufgabe kann nicht geschlossen werden" hinzufügen. Dies können Sie nicht mit einer Problemumgehung lösen. Sie benötigen ein anderes Modell, das nicht gegen LSP verstößt (oder wir müssen die Einschränkung "Voraussetzungen können nicht verschärft werden" lösen).

Sie können das Dekorationsmuster ausprobieren, wenn Sie in diesem Fall eine LSP-Verletzung vermeiden möchten. Es könnte funktionieren, ich weiß es nicht.

inf3rno
quelle