Запах логического кода проверки бизнеса

Question

Рассмотрим следующий код:

partial class OurBusinessObject {
    partial void OnOurPropertyChanged() {
        if(ValidateOurProperty(this.OurProperty) == false) {
            this.OurProperty = OurBusinessObject.Default.OurProperty;
        }
    }
}

То есть, когда значение OurProperty в OurBusinessObject изменяется, если значение недопустимо, установите его в качестве значения по умолчанию. Этот шаблон кажется мне запахом кода, но другие здесь (у моего работодателя) не согласны. Что ты думаешь?

Отредактировано, чтобы добавить : меня попросили добавить объяснение, почему это считается нормальным. Идея заключалась в том, что вместо того, чтобы производители бизнес-объекта проверяли данные, бизнес-объект мог проверять свои собственные свойства и устанавливать чистые значения по умолчанию в случаях, когда проверка не удалась. Кроме того, считалось, что если правила проверки изменятся, производителям бизнес-объектов не придется менять свою логику, поскольку бизнес-объект позаботится о проверке и очистке данных.

Answer 1

Это абсолютно ужасно. Удачи в устранении проблем в Production. Единственное, к чему это может привести, - это скрыть ошибки, которые просто всплывают где-то еще, где вообще не будет очевидно, откуда они берутся.

Answer 2

Для меня это похоже на симптом, а не на реальную проблему. На самом деле происходит то, что установщик для OurProperty не может сохранить исходное значение для использования в событии OnOurPropertyChanged. Если вы сделаете это, внезапно станет легче делать лучший выбор, как поступить.

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

Answer 3

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

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

Answer 4

Определенно сомнительная практика.

Как недопустимое значение может быть присвоено этому свойству? Разве это не означает, что где-то в вызывающем коде есть ошибка, и в этом случае вы, вероятно, захотите узнать об этом сразу? Или что пользователь вводит что-то неправильно, и в этом случае они должны быть немедленно проинформированы?

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

Answer 5

Я бы настоятельно рекомендовал сделать рефакторинг для проверки перед установкой свойства.

У вас всегда мог быть метод, который был бы больше похож на:

T GetValidValueForProperty<T>(T suggestedValue, T currentValue);

или даже:

T GetValidValueForProperty<T>(string propertyName, T suggestedValue, T currentValue);

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

Это будет использоваться больше как:

T OurProperty { get { return this.propertyBackingField; } set { this.propertyBackingField = this.GetValidValueForProperty(value, this.propertyBackingField); } }

Неважно, что вы делаете, но важно проверить, прежде чем изменить текущее значение. Если вы измените свое значение до того, как определите, является ли новое значение хорошим, вы напрашиваетесь на неприятности в долгосрочной перспективе.

Answer 6

Если не принимать во внимание термин «запах кода», вы можете быть правы - в зависимости от того, откуда он берется, тихое изменение значения, вероятно, не очень хорошая вещь. Было бы лучше убедиться, что ваше значение действительно, а не просто возвращаться к значению по умолчанию.

Answer 7

Я бы сказал, реализовать PropertyChanging и позволить бизнес-логике утверждать / отклонять значение, а затем генерировать исключение для недействительных значений.

Таким образом, у вас никогда не будет недопустимого значения. Это, и вы никогда не должны изменять информацию пользователя. Что если пользователь добавляет запись в базу данных и отслеживает ее для своих собственных записей? Ваш код переназначит значение по умолчанию, и теперь он отслеживает неверную информацию. Лучше сообщить пользователю как можно скорее.

Answer 8

Аргумент, который я имею против этого, заключается в следующем. Предположим, что пользователь / производитель бизнес-объекта случайно вводит недопустимое значение. Затем этот шаблон замаскирует этот факт и по умолчанию очистит данные. Но правильный способ справиться с этим - выдать ошибку и попросить пользователя / производителя проверить / очистить свои входные данные.

Answer 9

Я согласен с Grzenio и хотел бы добавить, что лучший способ обработки ошибки валидации на уровне домена (он же бизнес-объекты) - генерировать исключение. Это исключение может распространяться вплоть до уровня пользовательского интерфейса, где оно может быть обработано и интерактивно исправлено пользователем. Однако, в зависимости от задействованных возможностей и технологий, это не всегда возможно, и в этом случае вам, вероятно, следует выполнить проверку на уровне пользовательского интерфейса (возможно, в дополнение к уровню домена). Это менее чем идеально, но может быть вашим единственным жизнеспособным вариантом. В любом случае, установка значения по умолчанию - ужасная вещь, которая приведет к незначительным ошибкам, которые почти невозможно будет диагностировать. Если это будет сделано в широких масштабах, у вас мгновенно появится неработоспособная система (особенно если у вас нет юнит-тестов, поддерживающих вас).

Answer 10

Кроме того, почему в мире это частичное? Использование частичных классов для всего, кроме структуры сгенерированного кода, само по себе является кодексом, так как вы, вероятно, используете их, чтобы скрыть сложность, которую в любом случае следует разделить!