Czy jest to naruszenie zasady substytucji Liskowa?

132

Powiedzmy, że mamy listę encji Zadania i ProjectTaskpodtyp. Zadania można zamknąć w dowolnym momencie, z wyjątkiem tych, ProjectTasksktórych nie można zamknąć, gdy mają status Uruchomione. Interfejs użytkownika powinien upewnić się, że opcja zamknięcia uruchomionego ProjectTasknigdy nie jest dostępna, ale w domenie istnieją pewne zabezpieczenia:

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

Teraz podczas wywoływania Close()Zadania istnieje szansa, że ​​połączenie zakończy się niepowodzeniem, jeśli ma ProjectTaskstatus Rozpoczęty, a nie będzie, jeśli będzie to zadanie podstawowe. Ale to są wymagania biznesowe. To powinno zawieść. Czy można to uznać za naruszenie zasady substytucji Liskowa ?

Paul T. Davies
źródło
14
Idealny do przykładu T naruszenia zasady substytucji Liskowa. Nie używaj dziedziczenia tutaj, a wszystko będzie dobrze.
Jimmy Hoffa
8
Możesz to zmienić na public Status Status { get; private set; }:; w przeciwnym razie Close()można obejść tę metodę.
Job
5
Może to tylko ten przykład, ale nie widzę żadnych korzyści materialnych w związku z przestrzeganiem LSP. Dla mnie to rozwiązanie w pytaniu jest jaśniejsze, łatwiejsze do zrozumienia i łatwiejsze w utrzymaniu niż zgodne z LSP.
Ben Lee
2
@BenLee Nie jest łatwiej utrzymać. Tak to wygląda tylko dlatego, że widzisz to w izolacji. Gdy system jest duży, upewnienie się, że podtypy Tasknie wprowadzają dziwnych niezgodności w kodzie polimorficznym, o których tylko wie się, Taskto wielka sprawa. LSP nie jest kaprysem, ale został wprowadzony właśnie w celu ułatwienia konserwacji w dużych systemach.
Andres F.
8
@BenLee Wyobraź sobie, że masz TaskCloserproces, który closesAllTasks(tasks). Ten proces oczywiście nie próbuje wychwycić wyjątków; w końcu nie jest to część wyraźnej umowy z Task.Close(). Teraz wprowadzasz ProjectTaski nagle zaczynasz TaskCloserrzucać (prawdopodobnie nieobsługiwane) wyjątki. To wielka sprawa!
Andres F.

Odpowiedzi:

173

Tak, jest to naruszenie LSP. Wymaga tego zasada Liskowa

  • W podtypie nie można wzmocnić warunków wstępnych.
  • Warunki podrzędne nie mogą zostać osłabione w podtypie.
  • Niezmienniki nadtypu muszą być zachowane w podtypie.
  • Ograniczenie historii („reguła historii”). Obiekty są uważane za modyfikowalne tylko za pomocą ich metod (enkapsulacji). Ponieważ podtypy mogą wprowadzać metody, które nie są obecne w nadtypie, wprowadzenie tych metod może pozwolić na zmiany stanu podtypu, które nie są dopuszczalne w nadtypie. Ograniczenia historyczne tego zabraniają.

Twój przykład łamie pierwszy wymóg, wzmacniając warunek wstępny wywołania Close()metody.

Możesz to naprawić, przenosząc wzmocniony warunek wstępny na najwyższy poziom hierarchii dziedziczenia:

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

Zastrzegając, że wywołanie Close()jest ważne tylko w stanie, gdy CanClose()zwroty true, warunek wstępny stosuje się zarówno do, Taskjak i do ProjectTask, naprawiając naruszenie 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
źródło
17
Nie podoba mi się powielanie tego czeku. Wolałbym zgłaszanie wyjątków przechodząc do Zadania. Zamknij i usuń wirtualny z Zamknij.
Euforyczny
4
@Euphoric To prawda, że Closesprawdzanie na najwyższym poziomie i dodawanie chronionej DoClosebyłoby prawidłową alternatywą. Chciałem jednak pozostać jak najbliżej przykładu PO; poprawić to osobne pytanie.
dasblinkenlight,
5
@Euphoric: Ale teraz nie ma możliwości odpowiedzi na pytanie: „Czy to zadanie można zamknąć?” bez próby zamknięcia. To niepotrzebnie wymusza stosowanie wyjątków do kontroli przepływu. Przyznaję jednak, że tego rodzaju rzeczy można posunąć za daleko. Mówiąc zbyt daleko, tego rodzaju rozwiązanie może doprowadzić do bałaganu w przedsiębiorstwie. Niezależnie od tego pytanie PO wydaje mi się bardziej na temat zasad, więc odpowiedź z wieży z kości słoniowej jest bardzo odpowiednia. +1
Brian
30
@Brian The CanClose wciąż tam jest. Nadal można go wywołać, aby sprawdzić, czy zadanie można zamknąć. Odprawa w pobliżu powinna również to wywoływać.
Euforyczny
5
@Euphoric: Ach, źle zrozumiałem. Masz rację, dzięki czemu rozwiązanie jest znacznie czystsze.
Brian
82

Tak. Narusza to LSP.

Moją sugestią jest dodanie CanClosemetody / właściwości do zadania podstawowego, aby każde zadanie mogło stwierdzić, czy zadanie w tym stanie można zamknąć. Może również podać powód. I usuń wirtualny z Close.

Na podstawie mojego komentarza:

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);
    }
}
Euforyk
źródło
3
Dzięki za to poszedłeś o krok dalej od przykładu dasblinkenlight, ale podobało mi się jego wyjaśnienie i uzasadnienie. Niestety nie mogę zaakceptować 2 odpowiedzi!
Paul T Davies
Interesuje mnie, dlaczego podpis jest publicznym wirtualnym boolem CanClose (nasz ciąg znaków) - używając go, czy jesteś tylko zabezpieczeniem na przyszłość? A może brakuje mi czegoś bardziej subtelnego?
Reacher Gilt
3
@ReacherGilt Myślę, że powinieneś sprawdzić, co robią / ref i ponownie przeczytać mój kod. Jesteś zmieszany. Po prostu „Jeśli zadania nie można zamknąć, chcę wiedzieć, dlaczego”.
Euforyczny
2
out nie jest dostępny we wszystkich językach, zwracanie krotki (lub zwykłego obiektu zawierającego powód i wartość logiczną sprawiłoby, że byłby on bardziej przenośny w różnych językach OO, aczkolwiek kosztem utraty łatwości bezpośredniego posiadania boola. To powiedziawszy, dla języków, które DO wspierajcie, nic złego w tej odpowiedzi
Newtopian
1
I czy można wzmocnić warunki wstępne dla właściwości CanClose? Czyli dodajesz warunek?
John V
24

Zasada podstawienia Liskowa stanowi, że klasa podstawowa powinna być wymienna na dowolną z jego podklas bez zmiany jakichkolwiek pożądanych właściwości programu. Ponieważ tylko ProjectTaskpodnosi wyjątek po zamknięciu, program musiałby zostać zmieniony, aby pomieścił to, należy ProjectTaskgo zastąpić Task. To jest naruszenie.

Ale jeśli zmodyfikujesz Taskstwierdzając w swoim podpisie, że może powodować wyjątek po zamknięciu, nie naruszysz zasady.

Tulains Córdova
źródło
Używam c #, co nie wydaje mi się, że ma taką możliwość, ale wiem, że Java to robi.
Paul T Davies,
2
@PaulTDavies Możesz ozdobić metodę z wyjątkami, które zgłasza, msdn.microsoft.com/en-us/library/5ast78ax.aspx . Zauważysz to, gdy najedziesz kursorem na metodę z biblioteki klas podstawowych, otrzymasz listę wyjątków. Nie jest wymuszony, ale mimo to uświadamia dzwoniącemu.
Despertar,
18

Naruszenie LSP wymaga trzech stron. Typ T, podtyp S i program P, który używa T, ale otrzymuje instancję S.

Twoje pytanie zawiera T (Zadanie) i S (ProjectTask), ale nie P. Więc twoje pytanie jest niekompletne, a odpowiedź jest kwalifikowana: Jeśli istnieje P, które nie oczekuje wyjątku, to dla tego P masz LSP naruszenie. Jeśli każde P oczekuje wyjątku, oznacza to, że nie ma naruszenia LSP.

Jednak zrobić mają SRP naruszenie. Fakt, że stan zadania można zmienić, oraz polityka , zgodnie z którą niektóre zadania w niektórych stanach nie powinny być zmieniane na inne, to dwie bardzo różne obowiązki.

  • Odpowiedzialność 1: Reprezentuj zadanie.
  • Odpowiedzialność 2: Wdrożenie zasad zmieniających stan zadań.

Te dwie obowiązki zmieniają się z różnych powodów i dlatego należy je rozdzielić. Zadania powinny obsłużyć fakt bycia zadaniem oraz dane powiązane z zadaniem. TaskStatePolicy powinien obsłużyć sposób przejścia zadań ze stanu do stanu w danej aplikacji.

Robert Martin
źródło
2
Odpowiedzialność w dużej mierze zależy od domeny i (w tym przykładzie), jak złożone są stany zadań i ich zmieniacze. W tym przypadku nic nie wskazuje na to, więc nie ma problemu z SRP. Jeśli chodzi o naruszenie LSP, uważam, że wszyscy założyliśmy, że dzwoniący nie spodziewa się wyjątku, a aplikacja powinna wyświetlać rozsądny komunikat, zamiast przechodzić w błąd.
Euforii
Unca „Bob odpowiada? „Nie jesteśmy godni! Nie jesteśmy godni!”. W każdym razie ... Jeśli każde P oczekuje wyjątku, oznacza to, że nie ma naruszenia LSP. ALE jeśli zastrzegamy, że instancja T nie może wyrzucić OpenTaskException(podpowiedzi, podpowiedzi) i każde P oczekuje wyjątku, to co to znaczy o kodzie interfejsu, a nie implementacji? O czym mówię Nie wiem Jestem po prostu zaskoczony, że komentuję odpowiedź Unca 'Bob.
radarbob
3
Masz rację, że udowodnienie naruszenia LSP wymaga trzech obiektów. Jednak naruszenie LSP istnieje, jeśli istnieje jakikolwiek program P, który byłby poprawny w przypadku braku S, ale nie powiódł się po dodaniu S.
kevin cline
16

Może to, ale nie musi, stanowić naruszenie LSP.

Poważnie. Wysłuchaj mnie.

Jeśli podążasz za LSP, obiekty typu ProjectTaskmuszą zachowywać się tak, jak Taskoczekuje się od obiektów typu .

Problem z twoim kodem polega na tym, że nie udokumentowałeś, jak Taskpowinny zachowywać się obiekty typu . Napisałeś kod, ale żadnych umów. Dodam umowę na Task.Close. W zależności od dodanej przeze mnie umowy kod dla ProjectTask.CloseLSP jest zgodny z LSP lub nie.

Biorąc pod uwagę następującą umowę dla Task.Close, kod dla ProjectTask.Close nie jest zgodny z 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;
     }

Biorąc pod uwagę następującą umowę dla Task.Close, kod dla ProjectTask.Close jest zgodny z 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;
     }

Metody, które można zastąpić, należy udokumentować na dwa sposoby:

  • „Zachowanie” dokumentuje to, na czym może polegać klient, który wie, że obiekt adresata jest Task, ale nie wie, do której klasy jest bezpośrednią instancją. Informuje także projektantów podklas, które przesłonięcia są uzasadnione, a które nie.

  • „Zachowanie domyślne” dokumentuje to, na czym może polegać klient, który wie, że obiekt adresata jest bezpośrednim wystąpieniem Task(tj. Co otrzymujesz, jeśli go użyjesz new Task(). Mówi także projektantom podklas, jakie zachowanie zostanie odziedziczone, jeśli tego nie zrobi przesłonić metodę.

Teraz powinny obowiązywać następujące relacje:

  • Jeśli S jest podtypem T, udokumentowane zachowanie S powinno udoskonalić udokumentowane zachowanie T.
  • Jeśli S jest podtypem (lub równym) T, zachowanie kodu S powinno udoskonalić udokumentowane zachowanie T.
  • Jeśli S jest podtypem (lub równym) T, domyślne zachowanie S powinno udoskonalić udokumentowane zachowanie T.
  • Rzeczywiste zachowanie kodu dla klasy powinno udoskonalić jego udokumentowane zachowanie domyślne.
Theodore Norvell
źródło
@ user61852 podniósł kwestię, że można stwierdzić w podpisie metody, że może zgłosić wyjątek, a po prostu robiąc to (coś, co nie ma rzeczywistego kodu efektu), nie łamiesz już LSP.
Paul T Davies,
@PaulTDavies Masz rację. Ale w większości języków podpis nie jest dobrym sposobem na stwierdzenie, że procedura może zgłosić wyjątek. Na przykład w OP (myślę, że w C #) druga implementacja do Closerzuca. Więc podpis deklaruje, że wyjątek może zostać zgłoszony - nie oznacza, że ​​nie zostanie zgłoszony. Pod tym względem Java radzi sobie lepiej. Mimo to, jeśli zadeklarujesz, że metoda może zadeklarować wyjątek, powinieneś udokumentować okoliczności, w których może (lub będzie). Twierdzę więc, że aby mieć pewność, czy LSP zostanie naruszone, potrzebujemy dokumentacji wykraczającej poza podpis.
Theodore Norvell,
4
Wiele odpowiedzi tutaj całkowicie ignoruje fakt, że nie wiesz, czy umowa jest zatwierdzona, jeśli nie znasz umowy. Dziękuję za tę odpowiedź.
gnasher729
Dobra odpowiedź, ale pozostałe również są dobre. Wnioskują, że klasa podstawowa nie zgłasza wyjątku, ponieważ nic nie ma w tej klasie oznak tego. Dlatego program, który korzysta z klasy podstawowej, nie powinien przygotowywać się na wyjątki.
inf3rno 10.10.17
Masz rację, że lista wyjątków powinna być gdzieś udokumentowana. Myślę, że najlepsze miejsce jest w kodzie. Tutaj jest powiązane pytanie: stackoverflow.com/questions/16700130/... Ale możesz to zrobić bez adnotacji itp., Po prostu napisz coś podobnego if (false) throw new Exception("cannot start")do klasy podstawowej. Kompilator go usunie, a mimo to kod zawiera potrzebne informacje. Btw. nadal mamy naruszenie LSP przy tych obejściach, ponieważ warunek jest nadal wzmocniony ...
inf3rno 10.10.17
6

Nie stanowi to naruszenia zasady substytucji Liskowa.

Zasada substytucji Liskowa mówi:

Niech q (x) być właściwością dowodliwe o obiektach x typu T . Niech S jest podtypem T . Typ S narusza zasadę podstawienia Liskowa, jeśli istnieje obiekt y typu S , tak że q (y) nie da się udowodnić.

Powód, dla którego wdrożenie tego podtypu nie jest naruszeniem zasady substytucji Liskowa, jest dość prosty: nic nie można udowodnić na temat tego, co Task::Close()faktycznie robi. Jasne, ProjectTask::Close()zgłasza wyjątek podczas Status == Status.Started, ale tak może Status = Status.Closedsię Task::Close().

Oswald
źródło
4

Tak, to naruszenie.

Sugerowałbym, abyś miał swoją hierarchię do tyłu. Jeśli nie każdy Taskmożna zamknąć, to close()nie należy do Task. Być może potrzebujesz interfejsu, CloseableTaskktórego wszyscy nie ProjectTasksmogą wdrożyć.

Tom G.
źródło
3
Każde zadanie jest zamykane, ale nie w każdych okolicznościach.
Paul T Davies
Takie podejście wydaje mi się ryzykowne, ponieważ ludzie mogą pisać kod, oczekując, że wszystkie zadania wdrożą ClosableTask, choć dokładnie modeluje problem. Jestem rozdarty między tym podejściem a maszyną stanową, ponieważ nienawidzę maszyn stanowych.
Jimmy Hoffa
Jeśli Tasksam się nie implementuje CloseableTask, robią niebezpieczne miejsce, by nawet zadzwonić Close().
Tom G
@TomG to jest to, czego się boję
Jimmy Hoffa
1
Istnieje już automat stanowy. Nie można zamknąć obiektu, ponieważ jest w złym stanie.
Kaz.
3

Oprócz tego, że jest to problem LSP, wygląda na to, że używa wyjątków do kontrolowania przepływu programu (muszę założyć, że złapiesz gdzieś ten trywialny wyjątek i wykonasz jakiś niestandardowy przepływ, zamiast pozwolić, aby zawiesił twoją aplikację).

Wydaje się, że jest to dobre miejsce do wdrożenia wzorca stanu dla TaskState i umożliwienia obiektom stanu zarządzania prawidłowymi przejściami.

Ed Hastings
źródło
1

Brakuje mi tutaj ważnej rzeczy związanej z LSP i projektowaniem na podstawie umowy - w warunkach wstępnych to osoba dzwoniąca jest odpowiedzialna za upewnienie się, że warunki wstępne są spełnione. Kod wywoływany w teorii DbC nie powinien weryfikować warunku wstępnego. Umowa powinna określać, kiedy zadanie może zostać zamknięte (np. CanClose zwraca wartość True), a następnie kod wywołujący powinien zapewnić spełnienie warunku wstępnego przed wywołaniem funkcji Close ().

Ezoela Vacca
źródło
Umowa powinna określać dowolne zachowanie, którego potrzebuje firma. W takim przypadku funkcja Close () zgłosi wyjątek, gdy zostanie wywołana na początku ProjectTask. Jest to warunek końcowy (mówi o tym, co dzieje się po wywołaniu metody), a spełnienie go jest obowiązkiem wywoływanego kodu.
Goyo
@Goyo Tak, ale jak powiedzieli inni, wyjątek został podniesiony w podtypie, który wzmocnił warunek wstępny i tym samym naruszył (domyślny) kontrakt, który wywołuje zamknięcie zadania ().
Ezoela Vacca
Który warunek? Nie widzę żadnych.
Goyo
@ Gooy Sprawdź na przykład przyjętą odpowiedź :) W klasie podstawowej Close nie ma żadnych warunków wstępnych, jest wywoływany i zamyka zadanie. Jednak u dziecka istnieje warunek, aby status nie został rozpoczęty. Jak zauważyli inni, są to bardziej rygorystyczne kryteria, a zatem zachowanie nie może być zastąpione.
Ezoela Vacca
Nieważne, znalazłem warunek wstępny w pytaniu. Ale wtedy nie ma nic złego (pod względem DbC) z wywoływanym kodem sprawdzającym warunki wstępne i zgłaszającym wyjątki, gdy nie są one spełnione. Nazywa się to „programowaniem obronnym”. Ponadto, jeśli istnieje warunek końcowy stwierdzający, co się stanie, gdy warunek wstępny nie zostanie spełniony, jak w tym przypadku, wdrożenie musi zweryfikować warunek wstępny, aby upewnić się, że warunek końcowy jest spełniony.
Goyo
0

Tak, jest to wyraźne naruszenie LSP.

Niektórzy twierdzą tutaj, że wyraźne w klasie podstawowej, że podklasy mogą zgłaszać wyjątki, uczyniłoby to akceptowalnym, ale nie sądzę, że to prawda. Bez względu na to, co dokumentujesz w klasie podstawowej lub na jaki poziom abstrakcji przenosisz kod, warunki wstępne będą nadal wzmocnione w podklasie, ponieważ dodasz do niej część „Nie można zamknąć rozpoczętego zadania projektowego”. Nie jest to coś, co można rozwiązać za pomocą obejścia, potrzebujesz innego modelu, który nie narusza LSP (lub musimy poluzować ograniczenie „warunków wstępnych nie można wzmocnić”).

Możesz wypróbować wzór dekoratora, jeśli chcesz uniknąć naruszenia LSP w tym przypadku. To może zadziałać, nie wiem.

inf3rno
źródło