Как стать автором
Обновить

Комментарии 77

// тесты 

LoginService.setForTest(new MockLoginService()); // разве эта операция не дает возможность подменить настоящий LoginService на мок?

AdminDashboard adminDashboard = new AdminDashboard();
assertTrue(adminDashboard.isAuthenticatedAdminUser(user));
// нет способа подменить LoginService, будет использован настоящий
// жизнь боль
Да вы правы, не совсем верно, что нет способа подменить, LoginService.setForTest(...) даст возможность. Но такой подход к написанию тестов не является «правильным», о чем говорится в начале статьи:
Далее под понятием тест подразумевается красивый и чистый unit тест, который написан без различных хаков, без дополнительных «магических» библиотек для подмены зависимостей на лету, и прочего удовольствия, затрудняющего чтение и поддержку тестов.
Оххх…

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

var house = new House{Kitchen=new Kitchen()};

Это становиться неудобно если у вас количество полей\проперти больше пятака. Но при этом создавая конструктор с 7-8 аргументами одного типа вы нарываетесь на проблемы. Причин же не инициализировать в конструкторе по минимум две:

1) Вы имеете шанс нарваться на неожидаемое исключение, а писать что-то вида: try{}cath(Exception e){} — гробить всю логику обработки ошибок

2) Потенциальная ресурсоемкость и непредсказуемая длительность инициализации. Вот скажите, зачем мне при создании объекта формы тут же тащить данные из БД, подвешивая все и вся? Именно поэтому есть класс init который занимается сложной и тяжелой иницилизацией в специально отведенном ему месте.

Другой пример. Вам нужно скопировать объект. Что будет происходить если конструктор инициируется в конструкторе: сперва вы заполняете одними значениями. потом поверх них накатываете другие. Скажите: копируете через конструкторы с аргументами иницилизации? — добро пожаловать в отладку при добавлении нового поля\проперти. Скажите что это влияет на скорость выполнения? — А если у вас таких объектов с десяток миллионов?

Далее, если поля имеют ограничения в доступе, то расширять область доступа к ним через конструктор — нарушение инкапсуляции в рабочем коде. Причем при написании теста в C++ я не стесняюсь писать define private public и очень иногда жалею. что в C# этого не сделать и приходится выкручиваться

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

Вообще говоря писать тестируемый код просто — просто прежде чем стучать по клавишам надо задаться вопрос: а как я этот код буду контролировать.
Писать тестируемый код просто, если сначала написать тест для него :)
Тестируемый код не обязан иметь тесты. Можно лишь гипотетически представить как будет написан тест на класс. Если представить получилось — значит код тестируемый.
TDD — хорошая практика, но увы, ограниченная. Когда вы пишите код с довольно сложной логикой, который должен обрабатывать разнообразные ситуации, то итоговый код с 100% вероятностью будет выглядить сильно не так, как вы представляли вначале. При хорошей предварительной проработке класс идеологически не изменится. Вот на эти моменты вы можете изначально написать тесты, более подробные станут только обузой и вы будите больше времени тратить на правку тестов, чем логики.
TDD не исключает предварительную проработку интерфейсов. Более того, она её прямо предполагает.
Не все есть объект, соответственно не все определяется интерфейсом. Например на числомолотилку бесполезно писать подобные тесты
А почему лучше пользоваться конструкцией?
var house = new House{Kitchen=new Kitchen()};

Как минимум можно забыть / не знать про необходимость инициализации этого свойства и получить объект в несогласованном состоянии.

Это читабельнее, вы, не лезя в код конструктора, видите какие свойства заданы.

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

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

class House {
Kitchen kitchen;
Bedroom bedroom = new Bedroom();

House() {
this.kitchen = new Kitchen(new Refrigerator());
}

//…
}
Мы видим два момента: конструктор без параметров как таковых и два поля с модифактором доступа по умолчанию. Предлагаемое решение вот это:
class House {
Kitchen kitchen;
Bedroom bedroom;

House(Kitchen kitchen, Bedroom bedroom) {
this.kitchen = kitchen;
this.bedroom = bedroom;
}

//…
}
Плюс добавляется фабрика.

Теперь что говорю я:
По синтаксису C# и Java поля c модификатором доступа по умолчанию имеют ограничение в области видимости. Меняя сигнатуру конструктора вы даете доступ к ним извне. Нарушение инкапсуляции.

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

var house = new House { Kitchen = new Kitchen() }; 

Что нам мешает через десять строчек сунуть в house.Kitchen что-нибудь другое? Можно, конечно, сделать write-once property, но вообще это нарушение принципа наименьшего удивления.

Если зависимости передавать в конструкторе, они точно так же остаются видны, «недоинициализированность» невозможна. Есть и ещё один жирный плюс — IoC-контейнеры. Они очень любят конструкторы (де-факто стандарт реализации DI), а проперти как раз наборот не любят.
>>Я, может, чего-то не увидел, но вот в вашем примере я как раз и наблюдаю нарушение инкапсуляции:
У меня ее быть не может, потому-что я исхожу из того, что автор примера опечатался и поля имеют модификатор public

P.S. IoC-контейнеры не юзал, ничего сказать не могу
Так у автора же псевдокод, помесь C#, Java и TypeScript. Модификаторы доступа поставляйте мысленно, наиболее адекватные.
На typescript не довелось писать, а вот в C# и Java, как я уже говорил, модификаторы доступа имеют ограничения. Дальше по примеру видим:

House house = new House();
// ээээм, непонятно как тестировать
// нет доступа к kitchen и bedroom

Все как и говорил: поля с ограниченным доступом.
и поля имеют модификатор public

Судя по «нет доступа к kitchen и bedroom» они имеют модификатор private.
Вы имеете шанс нарваться на неожидаемое исключение
Не кидайте в конструкторах исключения ;)

зачем мне при создании объекта формы тут же тащить данные из БД, подвешивая все и вся?
А правда, зачем? Потребуйте в конструкторе «предоставлятор» данных, который знает, как тащить данные, но не тащит, пока не попросят. Когда данные действительно понадобятся, тогда, пожалуйста, тащите.
>>Не кидайте в конструкторах исключения ;)
В принципе невозможно, даже в C++ при нехватке памяти кидается исключение
>>А правда, зачем?
И вы предлагаете отложенную или ленивую инициализацию. В любом случае конструктор это уже не делает.
В принципе невозможно, даже в C++ при нехватке памяти кидается исключение
Ну, нехватка памяти — это в большинстве случаев уже капут. Быстро падаем и не думаем.

Вашему классу совершенно всё равно, как и откуда берутся данные. У него есть только провайдер. А берутся ли данные из кэша или их нужно грузить по сети из бд, установлено ли уже соединение с базой, нужно ли перед этим всем пожарить яичницу — это всё ненужные классу детали.
>>Ну, нехватка памяти — это в большинстве случаев уже капут. Быстро падаем и не думаем.
Или же штатно закрываем зарвавшийся модуль и предлагаем пользователю подождать и не ломать ему всю работу.
>>Вашему классу совершенно всё равно, как и откуда берутся данные.
И он по-прежнему не делает эту работу в конструкторе. С таким же успехом можно задать где-нибудь проперти типа Source.
тестируя код, да и в боевом коде лучше сразу видеть какие Поля\Свойства вы задаете, а не выяснять это залезая в код

В общем случае это прямое нарушение инкапсуляции. Клиенту класса не должно быть дела как класс реализует свои интерфейсы, у клиента не должно быть прямого доступа к состоянию, только через методы.
Да елы-палы, хватит меня считать шизофреником! Если я говорю, что доступ к «приватным» полям через конструктор нарушение инкапсуляции, это значит что я ее буду нарушать еще более грубым способом! Нет!

Я говорил, что если у нас изначально ситуация:

class House {
public Kitchen kitchen;
public Bedroom bedroom = new Bedroom();

public House() {
this.kitchen = new Kitchen(new Refrigerator());
}

То вместо вот этого:
class House {
public Kitchen kitchen;
public Bedroom bedroom = new Bedroom();

public House(Kitchen kitchen, Bedroom bedroom ) {

}
плюс фабрика, или даже этого:
class House {
public Kitchen kitchen;
public Bedroom bedroom = new Bedroom();

public House(Kitchen kitchen, Bedroom bedroom ) {

}
public House() {
this.kitchen = new Kitchen(new Refrigerator());
}

Лучше использовать исходный вариант, с конструкцией, которую я привел. И мне уже привели пример, что такая конструкция неудобна для IoC-контейнера
Не в контейнере дело. Передача параметров в конструктор не нарушает инкапсуляции — конструктор часть публичного интерфейса, мы лишь сообщаем что ему нужно передать такие объекты. Будут ли они присвоены свойствам (и есть ли именно такие свойства, а может другие), проигнорированы или отправлены в ФСБ, мы не знаем. Если присваивать свойства как у вас, то нужно знать об этих свойствах, их имена, типы, тупо не забыть присвоить при создании.
Дожили, пишем код и не знаем зачем пишем. А как проверять тогда будем?

Я, если честно, уже не понимаю, о чём спор.

public void Test()
{
    var kitchen = GetKitchenMock( ... );
    var bedroom = GetKitchenMock( ... );
    var house = new House(kitchen, bedroom);

    house.Burn();

    kitchen.IsBurnt.Should().BeTrue();
    bedroom.IsBurnt.Should().BeTrue();
}

Тут даже неважно, есть ли вообще у House свойства Kitchen и Bedroom.
А нафига вам это тестировать, если поля закрыты?????
Я тестирую корректное взаимодействие дома с кухней и спальней.
Ваш напор на закрытость полей мне не понятен. Ну вот я открою поля:

class House
{
    public Kitchen Kitchen { get; }
    public Bedroom Bedroom { get; }
    ...
}

В Java свойств нет, но там это решается отдельными геттерами:

public Kitchen getKitchen() { return mKitchen; }

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

У нас есть уже готовый класс со своим поведением. Также у нас изначально закрыты поля\свойства. И вся логика объекта этот факт учитывает.

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

Вот теперь вопрос: передавая произвольные параметры я точно тестирую нужное мне поведение?
Передавайте не произвольные параметры, а параметры, которые соответствуют тестируемым случаям.
Да каким, если они не подразумевались вовсе?
Что значит «не подразумевались»? Написали класс, который неизвестно что делает?
Нет, известно, но также известно, что для этой работы ему не нужно подавать на вход ни Kitchen, ни Bedroom.от слова вообще
Инстансы не нужно, а вот классы нужно по любому.

Не «не нужно», а «не нужно было». Да, передача аргументов в конструктор формально изменяет публичный интерфейс, но не нарушает инкапсуляции — мы не знаем, что с этими параметрами делает конструктор, может он их игнорирует и продолжает создавать через new.

Вообще лично мне пример решения с House не очень нравится, поскольку мы перенесли инстанцирование в фабрику, но не избавились от зависимости от конкретной реализации, просто перенесли её.
Интересный взгляд, чем-то напоминает СТО или квантовую механику: для пользователя класса инкапсуляция не нарушена, т.к. он не знает что именно делает класс, а вот с точки зрения автора конструктора инкапсуляция нарушена, т.к. он знает что именно делает конструктор.
Для автора не инкапсуляция нарушена, а изменены публичные интерфейсы, контракты и реализация.
Итого: для контроля, надобность в общем случае сомнительна, нам предлагается менять поведение класса. А делается это для того, чтобы протестировать оное.
Вообще говоря, такие изменения — это улучшение архитектуры в целом, а улучшение удобства тестируемости лишь следствие этого. В частности, перенося из конструктора класса конструирование своих объектов в фабрику, мы следуем (вольно или невольно — другой вопрос) принципу единственной ответственности: класс больше не ответственен за конструирование своих зависимостей, это теперь забота другого класса.
И вот тут нарушаем KISS
Принцип единственной ответственности — прямое следствие KISS. Конструктор класса конструирует свой инстанс — это просто. Конструктор класса конструирует свой инстанс и ещё с пяток других — это сложно.
Вообще лично мне пример решения с House не очень нравится, поскольку мы перенесли инстанцирование в фабрику, но не избавились от зависимости от конкретной реализации, просто перенесли её.

Для того, чтобы добиться тестируемости (а именно это было целью) не обязательно избавляться от конкретной реализации. Если бездумно скрывать все классы за интерфейсами, то ни к чему, кроме разбухания кодовой базы и усложнению поддержки, это не приведет.
Вам не нравится, что изначально Дом сам создавал себе Кухню и Спальню, а потом эти комнаты стали пихать извне?
Да, но не забудьте, что примеры условны.

Например, для моих любимых числомолотилок приватное поле может содержать некий буфер с предвычисленными значениями, которые должны быть такими и только такими, а вы подсовываете ему совсем другие.
Вы не видите разницы между буфером с какими-то значениями (= данные) и Кухней со Спальней (= сущности с логикой)?
Сущности с логикой? Где об этом сказано?
вызов любого метода House приведет к вызову kitchen и/или bedroom, а их мы не контролируем. Если они общаются с БД или шлют запросы в сеть, то тесты обречены.
Это раз.

Два, для тестируемости кода в этом случае применили внедрение зависимостей: не House управляет временем жизни Kitchen/Bedroom, а код извне, т.е. использующий House. Внедрение зависимостей и инверсия управления — это про логику. Данные не внедряют.
Ну можно и так понять, но принцип подается-то как универсальный.

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

Два, поля объявлены зависимостями и на основании чего — не понятно. Как они могут быть зависимостями, если методы класса не дают доступ к ним?

Методы не дают, но без этих классов класс House невозможно, как минимум, инстанцировать — он внутри себя обращается к, как минимум, конструкторам этих классов.
Ну и что с того?
А то, что он от них зависит. Скажем, скорость выполнения конструктора House зависит от скорости выполнения конструктора Bedroom.
Хотя, есть вариант как они могут стать зависимостями — это опять ход через глобальное состояние
Что синглтон, что глобальная переменная, оба являются глобальным состоянием. Именно наличие глобального состояния в коде усложняет его тестирование. И да, если у вас в коде есть синглтон, значит у вас в любом случае есть и глобальная переменная, пусть даже она называется «приватным статическим полем класса».
Это правда, но в синглетоне им можем адекватно управлять. Адекватный синглетон должен распознать, что он инициируется в тестовом окружении.
А мне кажется, что «боевой» код не должен ничего знать про тесты, а тем более распознавать что он запущен в тесте.
Это можно сделать и не занося тестовый код в боевой.
Если у вас получается такой сложный синглтон, который умеет распознавать, что он находится в тестовом окружении, и из-за этого, как я понимаю, может инициализироватся каким-то иным образом, может стоит вообще отказатся от идеи использования синглтона?

Допустим вы хотите сделать коннект с базой данных синглтоном, и хотите, чтобы этот умный синглтон подключался к боевой или тестовой БД в зависимости от окружения. Разве не лучше в таком случае просто инициализировать сервисы, которым нужно подключение к базе данных, передавая им в качестве аргумента объект коннектора, и вообще отказатся от идеи, что этот объект должен быть именно синглтоном? Просто в боевом окружении вы будете инициализировать сервисы с боевым коннектом к БД, а для тестов – с тестовым коннектом. На мой взгляд, такой подоход гибче (можно создавать сколько угодно способов и окружений для работаты с БД, например отдельно для нагрузочного тестирования и для stage), т.к. уменьшает связанность (сервис не завязан строго на конкртеных класс подключения) и избавляет от неявного поведения, заключенном в умном синглтоне (что бы изменить работы сервиса, нужно что-то перещелкнуть в другом месте).
Сразу оговорюсь, что по моему мнению те кто пишет на C# и использует синглтон для доступа к базе заместо Linq to Entity или, если .Net < 4.0, Linq to SQL — того багром и без лишних базаров. Поэтому пример не очень удачный, но давайте оставим тот момент, когда надо использовать этот патерн, а примем за аксиому, что он у нас есть.

Давайте возьмем класс Settings в C# (статический класс, да не синглтон, но это не суть), который читает настройки из App.config. Для боевого приложения берутся настройки, что лежат в боевом проекте, для теста — в тестовом. Сложно? — Абсолютно нет.

Более абстрактный пример, но для C++. Синглтон — класс из динамически подгружаемой либы. В боевом варианте система сборки кладет боевой вариант, в тестовом — тестовый. Сложно? — Не более чем все остальное в С++.

Мой пример не имеет никакого отношения конктрено к C# и это вымышленный пример, так что не придерайтесь.

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

С другой стороны, если вы хотите гарантировать уникальность чего-то в рамках приложения и его логики (которую мы и хотели бы потенциально протестировать), то синглтон – плохое решение. Допустим мы пишем WSGI-сервис на Python. Несколько различных сервисов могут сосуществовать в рамках одного процеса, и наоборот – один сервис может форкнутся до нескольких процессов. В обоих случаях из-за синглтона у нас проблемы.
Ну так кто бы спорил, что синглтон применяется не правильно в 99% случаях. Собственно о чем я и писал в тредстартовом посте.
(статический класс, да не синглтон, но это не суть)
Суть. Статический класс (а также реализация синглтона в виде Singleton.DefaultInstance) — это implicit dependency. Извне класса (читайте: по конструктору) не узнать, что класс использует какие-то там настройки.

Отдать управление временем жизни объекта на откуп IoC-контейнеру — сложно? Нет. Сам класс ничего не будет знать о том, в скольких экземплярах его насоздавали (зачем ему это?), а IoC-контейнер может гарантировать существование единственного экземпляра.
Синглетон с параметрами при создании? Мое удивление стремительно набирает вторую космическую и покидает орбиту старушки Земли.
Я нигде не говорил о параметрах.
Тогда я не улавливаю разницу между:
>>Статический класс (а также реализация синглтона в виде Singleton.DefaultInstance)
и моим: (статический класс, да не синглтон, но это не суть)

P.S. Или это как в старой книжке, мне стоило писать (статический класс — да, не синглтон, но это не суть)
Похоже, я начал какую-то мысль, а продолжил другой.
Читайте: это всё implicit dependency. И далее по комментарию.
И вот тут я понял, что потерял нить разговора
Всё-таки если в языке есть перегрузка функций, то можно и без большинства фабрик обойтись.
НЛО прилетело и опубликовало эту надпись здесь
Исключение:

оператор new можно использовать в бизнес коде для создания объектов-хранилищ, не содержащих поведения (например, HashMap, Array).
class Manager{
  ctro Manager( IProvider provider)

 public IEnumerable<Message> GetBy(...) {
  return m_provider.GetBy(...).Select(Convert);
 }

private Message Convert(DTO) {
  string s = DTO.Prop3.ToString();
  return new Message(DTO.Prop1, DTO.Prop2, s);
}

}


Message «не содержит» поведения.

Правильно ли делать метод Convert приватным в менеджере или стоит заинжектить в конструктор менеджера некий mocable MessageConverter?
Нормально ли использовать new в Convert?
Convert выглядит так будто он не связан с классом Manager, поэтому это должен быть некий отдельный MessageConverter. Так как Message «не содержит» поведения, то ничего плохого в создании его через new.
Правильно ли я трактую Ваш ответ? MessageConverter в методе которого new Message(… )?
Convert больше похож не на какой-то конвертер, а на фабричный метод и лучшим названием для него выглядит СreateFromDTO, а лучшим модификатором — static, тогда new Message вполне нормально смотрится и, в принципе, в ижектировании не нуждается (пока остается простым и быстрым). Или, как вариант, сделать его членом большой фабричного класса, например DomainFromDTOFabric.CreateMessage(DTO) и тогда уже инжектить.
Всем спасибо за ответы.
Я постарался сократить вопрос (пример) до минимума, чтобы не подводить к варианту, который я поддерживаю в споре с коллегой. Собственно, я за вариант, когда создание Message возлагается на отдельный класс.
Вариант со статикой мне не нравится (код снова останется в менеджере, не получится мокать (Moq)). А вот отдельная фабрика — это гуд!
Решил вот добавить еще одну маленькую деталь, которая добавляет (ИМХО) перевес в сторону инжектабл конвертера(фабрики).

class Manager{
  ctro Manager( IProvider provider, IFormatter formatter)

 public IEnumerable<Message> GetBy(...) {
  return m_provider.GetBy(...).Select(Convert);
 }

private Message Convert(DTO) {

  string s = m_formatter.ToPlaintext( DTO.Prop3 ); // IText Prop3

  return new Message(DTO.Prop1, DTO.Prop2, s);
}

}
Зарегистрируйтесь на Хабре, чтобы оставить комментарий