Это нарушение принципа подстановки Лискова?

133

Скажем, у нас есть список сущностей задач и ProjectTaskподтип. Задачи могут быть закрыты в любое время, кроме тех, ProjectTasksкоторые не могут быть закрыты, если они имеют статус Запущено. Пользовательский интерфейс должен гарантировать, что опция закрытия запуска ProjectTaskникогда не будет доступна, но в домене присутствуют некоторые меры безопасности:

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

Теперь при вызове Close()Задачи существует вероятность, что вызов не удастся, если он имеет ProjectTaskстатус «Запущено», а если нет, то если это была базовая Задача. Но это бизнес-требования. Это должно потерпеть неудачу. Можно ли считать это нарушением принципа подстановки Лискова ?

Пол Т Дэвис
источник
14
Идеальный пример нарушения подстановки лисков. Не используйте наследование здесь, и все будет в порядке.
Джимми Хоффа
8
Вы можете изменить это на public Status Status { get; private set; }:; в противном случае Close()метод можно обойти.
Работа
5
Может быть, это только этот пример, но я не вижу никакой материальной выгоды для соблюдения LSP. Для меня это решение в вопросе является более ясным, более простым для понимания и более легким в обслуживании, чем то, которое соответствует LSP.
Бен Ли
2
@BenLee Это не легче поддерживать. Это выглядит так только потому, что вы видите это изолированно. Когда система большая, удостовериться, что подтипы Taskне вносят причудливых несовместимостей в полиморфный код, о котором только известно, Task- это большая проблема. LSP не прихоть, но был введен именно для того, чтобы облегчить обслуживание в больших системах.
Андрес Ф.
8
@BenLee Представь, что у тебя есть TaskCloserпроцесс, который closesAllTasks(tasks). Этот процесс явно не пытается перехватить исключения; в конце концов, это не часть явного договора Task.Close(). Теперь вы вводите ProjectTaskи неожиданно TaskCloserначинаете генерировать (возможно необработанные) исключения. Это большое дело!
Андрес Ф.

Ответы:

174

Да, это нарушение LSP. Принцип замещения Лискова требует, чтобы

  • Предпосылки не могут быть усилены в подтипе.
  • Постусловия не могут быть ослаблены в подтипе.
  • Инварианты супертипа должны быть сохранены в подтипе.
  • Ограничение истории («правило истории»). Объекты считаются изменяемыми только через их методы (инкапсуляция). Поскольку подтипы могут вводить методы, которых нет в супертипе, введение этих методов может разрешить изменения состояния в подтипе, которые не допускаются в супертипе. Ограничение истории запрещает это.

Ваш пример нарушает первое требование, усиливая предварительное условие для вызова Close()метода.

Это можно исправить, перенеся усиленное предварительное условие на верхний уровень иерархии наследования:

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

Предусматривая, что вызов Close()действителен только в состоянии, при CanClose()возврате которого trueвы делаете предварительное условие, применимое как к, Taskтак и к ProjectTaskисправлению нарушения 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
источник
17
Мне не нравится дублирование этого чека. Я бы предпочел бросать исключения, входящие в Task.Close и удалять виртуальные из Close.
Эйфорическая
4
@Euphoric Это правда, что Closeпроверка на верхнем уровне и добавление защищенной DoCloseбудет допустимой альтернативой. Однако я хотел остаться как можно ближе к примеру ОП; улучшение это отдельный вопрос.
dasblinkenlight
5
@Euphoric: Но теперь нет возможности ответить на вопрос: «Может ли эта задача быть закрыта?» не пытаясь закрыть его. Это излишне заставляет использовать исключения для управления потоком. Я признаю, однако, что такого рода вещи можно зайти слишком далеко. Слишком далеко, решение такого рода может привести к предпринимательскому беспорядку. Несмотря на это, вопрос ОП мне больше нравится в принципах, поэтому ответ из башни из слоновой кости очень уместен. +1
Брайан
30
@ Брайан CanClose все еще там. Его все еще можно вызвать, чтобы проверить, можно ли закрыть задачу. Проверка в Close должна вызывать это тоже.
Эйфорическая
5
@ Эйфорик: Ах, я неправильно понял. Вы правы, это делает решение намного чище.
Брайан
82

Да. Это нарушает LSP.

Я предлагаю добавить CanCloseметод / свойство к базовой задаче, чтобы любая задача могла определить, можно ли закрыть задачу в этом состоянии. Это также может дать причину. И удалить виртуальный из Close.

Основываясь на моем комментарии:

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);
    }
}
Euphoric
источник
3
Спасибо за это, вы продвинули пример dasblinkenlight еще на один этап, но мне понравилось его объяснение и обоснование. Извините, я не могу принять 2 ответа!
Пол Т Дэвис
Мне интересно знать, почему подпись является общедоступной виртуальной булкой CanClose (из-за причин String) - от использования вы просто устремляетесь в будущее? Или я скучаю по чему-то более тонкому?
Ричер Гилт
3
@ReacherGilt Я думаю, вам следует проверить, что делает out / ref, и снова прочитать мой код. Ты смущен. Просто «Если задача не может быть закрыта, я хочу знать, почему».
Эйфорическая
2
out доступен не на всех языках, возвращая кортеж (или простой объект, инкапсулирующий причину и логическое значение, сделает его лучше переносимым на ОО-языки, хотя за счет потери простоты непосредственного использования bool. При этом для языков, которые СДЕЛАЮТ поддержать, ничего плохого в этом ответе.
Newtopian
1
И это нормально, чтобы усилить предварительные условия для свойства CanClose? Т.е. добавление условия?
Джон V
24

Принцип подстановки Лискова гласит, что базовый класс должен заменяться любым из его подклассов без изменения каких-либо желательных свойств программы. Поскольку ProjectTaskисключение возникает только при закрытии, программа должна быть заменена на acommodate для этого, должна ProjectTaskиспользоваться вместо Task. Так что это нарушение.

Но если вы измените, Taskуказав в своей подписи, что оно может вызвать исключение при закрытии, вы не нарушите этот принцип.

Тулаинс Кордова
источник
Я использую C #, который я не думаю, что есть такая возможность, но я знаю, что Java делает.
Пол Т Дэвис
2
@PaulTDavies Вы можете украсить метод с какими исключениями он выдает, msdn.microsoft.com/en-us/library/5ast78ax.aspx . Вы замечаете это, когда наводите указатель мыши на метод из библиотеки базовых классов, вы получаете список исключений. Он не применяется, но, тем не менее, информирует вызывающего абонента.
Despertar
18

Нарушение LSP требует трех сторон. Тип T, Подтип S и программа P, которая использует T, но получает экземпляр S.

Ваш вопрос предоставил T (Task) и S (ProjectTask), но не P. Таким образом, ваш вопрос является неполным и ответ квалифицирован: если существует P, который не ожидает исключения, тогда для этого P у вас есть LSP нарушение. Если каждый P ожидает исключения, то нарушения LSP нет.

Тем не менее, вы делаете иметь SRP нарушение. Тот факт, что состояние задачи может быть изменено, и политика, согласно которой определенные задачи в определенных штатах не должны быть изменены на другие состояния, являются двумя совершенно разными обязанностями.

  • Ответственность 1: Представьте задачу.
  • Ответственность 2: Реализация политик, которые изменяют состояние задач.

Эти две обязанности меняются по разным причинам и поэтому должны быть в разных классах. Задачи должны обрабатывать факт выполнения задачи и данные, связанные с задачей. TaskStatePolicy должен обрабатывать способ перехода задач из состояния в состояние в данном приложении.

Роберт Мартин
источник
2
Обязанности в значительной степени зависят от домена и (в этом примере) от того, насколько сложны состояния задачи и как они меняются. В этом случае нет указаний на такую ​​вещь, поэтому нет проблем с SRP. Что касается нарушения LSP, я полагаю, что мы все предполагали, что вызывающая сторона не ожидает исключения, и приложение должно показывать разумное сообщение вместо того, чтобы войти в ошибочное состояние.
Эйфорическая
Unca 'Боб отвечает? «Мы не достойны! Мы не достойны!». Во всяком случае ... Если каждый P ожидает исключения, то нет нарушения LSP. НО, если мы оговорим, что экземпляр T не может выдать OpenTaskException(подсказку, подсказку), и каждый P ожидает исключения, то что это говорит о коде для интерфейса, а не о реализации? О чем я говорю? Я не знаю. Я просто в восторге от того, что комментирую ответ Unca 'Bob.
Радар Боб
3
Вы правы, что для доказательства нарушения LSP требуется три объекта. Тем не менее, нарушение LSP существует, если существует ЛЮБАЯ программа P, которая была правильной в отсутствие S, но терпит неудачу с добавлением S.
kevin cline
16

Это может или не может быть нарушением LSP.

Шутки в сторону. Выслушайте меня.

Если вы следуете LSP, объекты типа ProjectTaskдолжны вести себя так, как Taskожидают себя объекты типа .

Проблема с вашим кодом в том, что вы не задокументировали, как Taskдолжны вести себя объекты типа . Вы написали код, но без контрактов. Я добавлю контракт на Task.Close. В зависимости от контракта, который я добавляю, код ProjectTask.Closeлибо соответствует, либо не соответствует LSP.

Учитывая следующий контракт для Task.Close, код для ProjectTask.Close не соответствует 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;
     }

Учитывая следующий контракт для Task.Close, код для ProjectTask.Close действительно следует за 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;
     }

Методы, которые могут быть переопределены, должны быть задокументированы двумя способами:

  • «Поведение» документирует, на что может положиться клиент, который знает, что объект-получатель является Task, но не знает, к какому классу он относится напрямую. Он также сообщает дизайнерам подклассов, какие переопределения являются разумными, а какие нет.

  • «Поведение по умолчанию» документирует, на что может положиться клиент, который знает, что объект-получатель является прямым экземпляром Task(т.е. что вы получаете, если вы используете new Task(). Он также сообщает дизайнерам подклассов, какое поведение будет унаследовано, если они этого не сделают переопределить метод.

Теперь должны соблюдаться следующие отношения:

  • Если S является подтипом T, документированное поведение S должно уточнить документированное поведение T.
  • Если S является подтипом (или равен) T, поведение кода S должно уточнять документированное поведение T.
  • Если S является подтипом (или равен) T, поведение S по умолчанию должно уточнить документированное поведение T.
  • Фактическое поведение кода для класса должно улучшить его документированное поведение по умолчанию.
Теодор Норвелл
источник
@ user61852 поднял вопрос о том, что в сигнатуре метода можно указать, что он может вызвать исключение, и, просто делая это (что не имеет смысла в действительном коде эффекта), вы больше не нарушаете LSP.
Пол Т Дэвис
@PaulTDavies Вы правы. Но в большинстве языков подпись не является хорошим способом объявить, что процедура может вызвать исключение. Например, в OP (в C #, я думаю) вторая реализация Closeдействительно выбрасывает. Таким образом, подпись заявляет, что может быть выдано исключение, но не говорит, что этого не произойдет. Java делает лучшую работу в этом отношении. Тем не менее, если вы объявляете, что метод может объявлять исключение, вы должны задокументировать обстоятельства, при которых он может (или будет). Поэтому я утверждаю, что для того, чтобы быть уверенным в том, что LSP нарушен, нам нужна документация за пределами подписи.
Теодор Норвелл
4
Множество ответов здесь, кажется, полностью игнорируют тот факт, что вы не можете знать, подтвержден ли контракт, если вы не знаете контракт. Спасибо за этот ответ.
gnasher729
Хороший ответ, но другие ответы также хороши. Они делают вывод, что базовый класс не выдает исключение, потому что в этом классе нет ничего, что демонстрировало бы признаки этого. Поэтому программа, использующая базовый класс, не должна готовиться к исключениям.
inf3rno
Вы правы, что список исключений должен быть где-то задокументирован. Я думаю, что лучшее место в коде. Здесь есть связанный вопрос: stackoverflow.com/questions/16700130/… Но вы можете сделать это без аннотаций и т. Д., А также просто написать что-то вроде if (false) throw new Exception("cannot start")базового класса. Компилятор удалит это, и все же код содержит то, что необходимо. Btw. у нас все еще есть нарушение LSP с этими обходными путями, потому что предварительное условие все еще усилено ...
inf3rno
6

Это не нарушение принципа подстановки Лискова.

Принцип замещения Лискова гласит:

Пусть д (х) является свойством доказуемо об объектах х типа Т . Пусть S является подтипом T . Тип S нарушает принцип подстановки Лискова, если существует объект y типа S , так что q (y) недоказуемо.

Причина, по которой ваша реализация подтипа не является нарушением принципа подстановки Лискова, довольно проста: ничто не может быть доказано о том, что на Task::Close()самом деле делает. Конечно, ProjectTask::Close()выдает исключение, когда Status == Status.Started, но может и Status = Status.Closedв Task::Close().

Освальд
источник
4

Да, это нарушение.

Я бы предложил, чтобы у вас была иерархия в обратном направлении. Если не каждый Taskзакрывается, то close()не принадлежит Task. Возможно, вы хотите интерфейс, CloseableTaskкоторый все не ProjectTasksмогут реализовать.

Том Г
источник
3
Каждая задача закрыта, но не при любых обстоятельствах.
Пол Т Дэвис
Этот подход кажется мне рискованным, так как люди могут писать код, ожидая, что все Task реализуют ClosableTask, хотя он точно моделирует проблему. Я разрываюсь между этим подходом и конечным автоматом, потому что я ненавижу конечные автоматы.
Джимми Хоффа
Если Taskсам не реализует, CloseableTaskто они делают небезопасный бросок куда-то, чтобы даже позвонить Close().
Том Г
@ TomG вот чего я боюсь
Джимми Хоффа
1
Уже есть конечный автомат. Объект не может быть закрыт, потому что он находится в неправильном состоянии.
Каз
3

В дополнение к проблеме LSP, кажется, что он использует исключения для управления потоком программы (я должен предположить, что вы где-то ловите это тривиальное исключение и делаете некоторый пользовательский поток, а не позволяете ему аварийно завершить работу приложения).

Похоже, что это хорошее место для реализации шаблона State для TaskState и позволяющего объектам состояния управлять допустимыми переходами.

Эд Гастингс
источник
1

Здесь я упускаю важную вещь, связанную с LSP и проектированием по контракту - в предварительных условиях именно вызывающий абонент должен убедиться, что предварительные условия выполнены. В теории DbC вызываемый код не должен проверять предварительное условие. В контракте должно быть указано, когда задача может быть закрыта (например, CanClose возвращает True), а затем вызывающий код должен убедиться, что предварительное условие выполнено, прежде чем вызывать метод Close ().

Эзоэла Вакка
источник
В контракте должно быть указано, какое поведение необходимо бизнесу. В этом случае этот Close () вызовет исключение при вызове запущенного ProjectTask. Это постусловие (оно говорит, что происходит после вызова метода), и его выполнение является обязанностью вызываемого кода.
Гойо
@Goyo Да, но, как говорили другие, в подтипе возникает исключение, которое усиливает предварительное условие и, таким образом, нарушает (подразумеваемый) контракт, согласно которому вызов Close () просто закрывает задачу.
Эзоела Вакка
Какие предварительные условия? Я не вижу ни одного.
Гойо
@Goyo Проверьте принятый ответ, например :) В базовом классе у Close нет предварительных условий, он вызывается и закрывает задачу. Однако у ребенка есть предварительное условие о том, что статус не начат. Как отмечали другие, это более строгие критерии, и поэтому поведение не поддается замене.
Эзоела Вакка
Неважно, я нашел предварительное условие в вопросе. Но тогда нет ничего плохого (с точки зрения DbC) в том, что вызываемый код проверяет предварительные условия и вызывает исключения, когда они не выполняются. Это называется "защитное программирование". Кроме того, если существует постусловие, указывающее, что происходит, когда предусловие не выполняется, как в этом случае, реализация должна проверять предусловие, чтобы гарантировать, что постусловие выполнено.
Гойо
0

Да, это явное нарушение LSP.

Некоторые люди утверждают, что в базовом классе указание на то, что подклассы могут генерировать исключения, сделало бы это приемлемым, но я не думаю, что это правда. Независимо от того, что вы документируете в базовом классе или на какой уровень абстракции вы перемещаете код, предварительные условия все равно будут усилены в подклассе, поскольку вы добавляете в него часть «Не удается закрыть запущенную задачу проекта». Это не то, что вы можете решить с помощью обходного пути, вам нужна другая модель, которая не нарушает LSP (или нам нужно ослабить ограничение «предварительные условия не могут быть усилены»).

Вы можете попробовать шаблон декоратора, если вы хотите избежать нарушения LSP в этом случае. Это может сработать, я не знаю.

inf3rno
источник