Functional C#: Primitive obsession (одержимость примитивами)

[@vladimirkolyada]

Не совсем корректный пример с логикой проверки на уровне ViewModel MVC, там логика атрибутов пролезла потому, что по ней строится клиентская валидация автоматизированно умеет. Я действую наоборот, на границе, перед входом в домен, проверяю и валидирую все, атрибутами и дополнительными валидаторами, по необходимости, а дальше, в домене, уже оперирую чистыми моделями, считая, что они валидацию прошли и корректны. Допустим, если я получаю по ключу значение из какого-то хранилища, где он должен быть, то я делаю First(), а не FirstOrDefault(). Т.е. другими словами гарантированно генерирую исключение. Но подход с логикой валидации в одном месте исключительно правильный, надо к нему стремиться. Но я его вижу на базе атрибутов + обработчика этих правил (универсального) без реализации одного и того же в виде списка if — ов и так далее.

[@vladimirkolyada]

Вдогонку, не успел отредактировать, прекрасный цикл статей намечается, продолжайте, пожалуйста!

[@Quanzi]

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

[@vladimirkolyada]

У меня нет личных проектов, вся разработка от 2 до 15 человек в команде. Но я с вами полностью согласен, бывает — забывают, но пишутся UT, где быстро вспоминают, все же редкость какая-то сложная логика валидации, в основном это наличие, промежуток, даже регулярка редкость. Да, представление действительно должно знать о том, какие правила валидации есть в домене, по одной простой причине, с представлением работает человек и его не интересует моя внутренняя логика, архитектура, ему интересно как можно скорее заполнить формы и видеть свои нарушения сразу, а не после отправки данных. Приходится искать консенсус.

[@kekekeks]

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

[@Quanzi]

Хорошее покрытие логики тестами действиетльно должно помогать, да. А по поводу второй части — не лучше ли, чтобы представление знало о наличии валидации, но сама валидация была скрыта внутри домена? Опять же, валидация при каждом обращении к домену это явное нарушение DRY (если каждый объект не вызывается ровно один раз).
Хотя, возможно, у вас хватает и такого способа, раз валидация не очень сложная.

[@impwx]

Пример с Customer и методами-сеттерами, имхо, высосан из пальца. Почему бы просто не разместить валидацию в коде сеттера свойства?

[@vladimirkolyada]

Мне кажется плохим наделять Setter такими свойствами валидации. Какая-то двуответственность (и более) получается. Но я не автор, может он что-то иное скажет. Но мне подсознательно видится это плохим решением.

[@impwx]

Отнюдь, как раз для этого и придуманы сеттеры:

Properties have many uses: they can validate data before allowing a change; they can transparently expose data on a class where that data is actually retrieved from some other source, such as a database; they can take an action when data is changed, such as raising an event, or changing the value of other fields.

[@vladimirkolyada]

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

[@lair]

Вопрос в целесообразности использования такого подхода при каких-то цепочках валидации, зависимости одной проперти от другой и так далее.

В таких случаях надо не метод ChangeSmth делать, а разносить присвоение и валидацию. А если валидация свойства атомарна (не зависит от других свойств), то ее целесообразно делать в сеттере.

[@Sing]

> По мне сеттер должен быть сеттером.

Сеттер — это банальный метод, который запускается при попытке присвоить параметру значение. То есть, методы из примера — ChangeEmail и ChangeName — это лишние сущности, которые должны быть запрятаны в set, заодно избавив от двух проверок в конструкторе и приведя решение к DRY.

Также мне лично непонятно разночтение между приватными сеттерами параметров, но публичными ChangeEmail и ChangeName. Это ставит в ступор, как минимум.

[@vkhorikov]

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

[@vkhorikov]

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

[@darkdaskin]

Валидацию не всегда можно уместить в сеттер. Более-менее сложные модели часто имеют правила, распространяющиеся на несколько свойств.

Ситуацию с проверками в сеттере можно довести до абсурда:

class Range
{
    private int _min;
    private int _max;

    public int Min
    {
        get { return _min; }
        set { if(value > Max) throw new InvalidOperationException(); _min = value; }
    }

    public int Max
    {
        get { return _max; }
        set { if (value < Min) throw new InvalidOperationException(); _max = value; }
    }
}

Получаем класс, свойства которого надо задавать в определённом порядке. Мне подобный код как-то попадался, работать с ним было очень тяжело.

[@lair]

Если валидация захватывает несколько свойств, ее нужно выносить в отдельный метод, который вызывается после присвоения всех свойств (либо, если возможно — делать метод, который меняет несколько свойств разом).

[@impwx]

У вашего примера есть принципиальное отличие — вы допускаете, что объект может в какой-то момент быть инициализирован лишь частично, и следовательно быть непригодным к использованию. Тогда, как сказал lair выше, делается отдельный метод валидации, или атомарный сеттер для связанных свойств.

[@lair]

Обратите вниманите, что конструктор класса Email закрыт, так что единственный способ создать его экземпляр — использовать статический метод Create, который проводит всю необходимую валидацию.

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

[HttpPost]
public ActionResult CreateCustomer(CustomerInfo customerInfo)
{
    Result<Email> emailResult = Email.Create(customerInfo.Email);
    Result<CustomerName> nameResult = CustomerName.Create(customerInfo.Name);
 
    if (emailResult.Failure)
        ModelState.AddModelError(“Email”, emailResult.Error);
    if (nameResult.Failure)
        ModelState.AddModelError(“Name”, nameResult.Error);
    ...
}

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

Более строгая система типов. Компилятор работает на нас с удвоенной силой: теперь невозможно ошибочно присвоить свойству типа Email объект типа CustomerName, такой код не будет скомпилирован.

Но при этом для других свойств (скажем, City и Country) такой проверки нет, и теперь программисту нужно помнить про два разных механизма. Не ужас, но неудобно.

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

[@mayorovp]

По поводу биндинга — можно и универсальное решение написать, будет почти из коробки (почти — потому что в коробку-то надо самому положить).

[@lair]

Можно, вопрос усилий. Для классов со статическими фабриками это, скажем так, требует времени.

[@kekekeks]

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

[@vkhorikov]

Good points.

1) По поводу ручного биндинга — вы правы, здесь теряется часть встроенного функционала, который есть в ASP.NET. Это вопрос взвешивания «за» и «против». Для меня плюсы более выразительной доменной модели перевешивают минусы необходимости писать подобный код вручную. Для более простых проектов вполне можно отказаться от этого подхода и делать по старинке.

Но при этом для других свойств (скажем, City и Country) такой проверки нет, и теперь программисту нужно помнить про два разных механизма. Не ужас, но неудобно.

Если у City и Country есть какие-то более-менее сложные инварианты, то их тоже стоит обернуть в классы-обертки.

2) По поводу невалидных данных в БД — отличный point. Есть два распространенных подхода к проблеме. Первый — вместе с ужесточением инвариантов писать скрипты для миграции данных в БД, чтобы они соответствовали новым инвариантам. Второй — создавать отдельный класс, к примеру EmailStrong для хранения имейлов, инварианты в которых были ужесточены и не давать присваивать кастомерам объекты старого Email, только нового. Со временем, когда БД придет в соответствие с новыми требованиями, старый Email удаляется, новый EmailStrong переименовывется в Email. Второй вариант сложнее, я как правило пользуюсь первым.

[@lair]

Если у City и Country есть какие-то более-менее сложные инварианты, то их тоже стоит обернуть в классы-обертки.

Нет у них инвариантов, поэтому не будет классов-оберток, поэтому нельзя выработать привычку к type safety net.

Первый — вместе с ужесточением инвариантов писать скрипты для миграции данных в БД, чтобы они соответствовали новым инвариантам.

К сожалению, это возможно далеко не всегда.

Второй — создавать отдельный класс, к примеру EmailStrong для хранения имейлов, инварианты в которых были ужесточены и не давать присваивать кастомерам объекты старого Email, только нового.

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

[@vkhorikov]

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

Обернуть можно. Где-то метод принимает старый Email, а где-то — только новый EmailStrong, который является наследником старого Email.

[@lair]

… и как понять, какой метод использовать в какой момент? А учитывая, что лучше все-таки использовать сеттеры, а не методы — так и вовсе весело.

[@vkhorikov]

Понять — по сигнатуре метода. Т.е. какие-то методы будут работать только с новыми имейлами, для других — мы оставляем как было.

лучше все-таки использовать сеттеры, а не методы

Здесь как раз пригодится сделать небольшие методы-сеттеры для обозначения новых инвариантов класса.

[@lair]

Т.е. какие-то методы будут работать только с новыми имейлами, для других — мы оставляем как было.

А что мешает использовать методы со старыми сигнатурами?

Здесь как раз пригодится сделать небольшие методы-сеттеры для обозначения новых инвариантов класса.

… и теперь сломался ранее работавший маппинг.

[@vkhorikov]

А что мешает использовать методы со старыми сигнатурами?

Ну мы же хотим чтобы кастомерам можно было присваивать только имейлы с новыми инвариантами? Если так, то можно на уровне компилятора обозначить это изменение.

… и теперь сломался ранее работавший маппинг.

Маппинг куда? Если речь про ORM, то они умеют памить на protected setter-ы. В EF это посложнее сделать, в NH — попроще.

[@lair]

Ну мы же хотим чтобы кастомерам можно было присваивать только имейлы с новыми инвариантами? Если так, то можно на уровне компилятора обозначить это изменение.

Хотим. Но если мы для обратной совместимости оставили возможность присвоить «старые» емейлы, как мы запретим программисту использовать эти механизмы?

Маппинг куда?

Куда угодно. На вью-модели, на DTO.

[@vkhorikov]

Запретить не получится. Но я бы поспорил с посылкой этого утверждения. Программисты-коллеги — друзья, а не враги, цель нового класса EmailStrong — не запретить им что-то делать, а подсказать, направить на правильный путь. Ну и плюс общение между коллегами должны быть intensive, чтобы все были в курсе нововведений.

[@lair]

Ну и плюс общение между коллегами должны быть intensive, чтобы все были в курсе нововведений.

К сожалению, чем больше проект (и коллектив), тем это сложнее.

[@withkittens]

как мы запретим программисту использовать эти механизмы?

Запретить не запретим, но если добавить запашку — Deprecated?

[@lair]

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

[@withkittens]

Result<Email>

Не является ли это анти-паттерном в C#? Такой тип идеален в Rust, там нет исключений, но в C# используются две модели — исключения и bool TryXXX(..., out yyy).

[@mayorovp]

Для задачи валидации данных — нет, не является. Исключения слишком дорогие чтобы вызывать их постоянно — а bool TryXXX(..., out yyy) возвращает слишком мало информации в случае ошибки.

[@withkittens]

Исключения — да, понятно. Меня смутил пример, уж больно Result похож на собрата из Rust.

[@vkhorikov]

Использовать исключения для валидации — плохая практика. Тут более подробно на эту тему: habrahabr.ru/post/263685

[@Trueteller]

Что-то я не понял: первую статью пишете про immutability, а вторая статья показывает mutable класс Customer, как до так и после рефакторинга… Зачем?

[@mayorovp]

Потому что вторая статья — не про неизменяемость…

[@mayorovp]

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

Но есть несколько проблем.
1. Задача «собрать все ошибки валидации» при таком подходе превращается в кошмар.
2. Если проверка валидности не раскладывается на отдельные проверки независимых полей — то все становится очень весело (выше был пример с границами диапазона).
3. Если значение свойства по умолчанию невалидно, то подход просто не работает.

[@PsyHaSTe]

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

[@lair]

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

Впрочем, предлагаемая в статье монада Try — это не код возврата, это пламенный привет от функционального програмирования и явных контрактов.

[@mayorovp]

Код возврата — это никому не понятное число… Result.Fail же принимает в качестве параметра строку на человеческом языке. Это не шаг назад, это шаг в сторону.

[@marshinov]

    [Required(ErrorMessage = “E-mail is required”)]
    [RegularExpression(@”^([\w\.\-]+)@([\w\-]+)((\.(\w){2,3})+)$”, ErrorMessage = “Invalid e-mail address”)]
    [StringLength(100, ErrorMessage = “E-mail is too long”)]
    public string Email { get; set; }

Этот код работал с ORM «из коробки». Можете привести пример как вы рекомендуете мапить обертки?

[@vkhorikov]

Здесь пример того как я обычно это делаю: github.com/vkhorikov/FuntionalPrinciplesCsharp/blob/master/New/CustomerManagement.Logic/Model/Email.cs

[@marshinov]

А если Customer'у потребуется менять email, то добавляем такое?

public void UpdatePrimaryEmail(Email email)
{
    _primaryEmail = email;   
}

public void UpdateSecondaryEmail(Email email)
{
    _seconaryEmail = email;   
}

public void ClearSeconaryEmail()
{
    _seconadryEmail = null;
}

[@vkhorikov]

Да, но не обязательно отдельными методами, property setters тоже подойдут.