Контракты, состояние и юнит-тесты

На примере кода Боба Мартина из его книги «Принципы, паттерны и методики гибкой разработки»

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

Описывая TDD Боб Мартин пишет о том, что TDD – это не просто вначале тесты, а потом код, это способ обдумывания дизайна класса через призму тестов, что делает его более обдуманным и слабосвязанным. Этот подход вполне оправдан и юнит тесты и правда являются отличной лакмусовой бумажкой качественного дизайна.

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

Я не предлагаю отказываться от юнит-тестов; это прекрасный способ покрытия граничных условий, добавления в код дополнительной документации и хороший способ проверки простоты повторного использования кода. Скорее, я предлагаю разработчикам, подумать о дизайне класса, с трех точек зрения; у класса есть требования к своим клиентам (предусловия методов), он что-то гарантирует своим клиентам (постусловия методов) и есть нечто, что является истинным на протяжении всего времени жизни объекта (инвариант объекта). И уже три эти аспекта проверять в юнит-тестах, а не пытаться с помощью тестов доказать существование этих трех аспектов. Чем четче и яснее мы сможем выразить эти концепции в нашем коде, тем проще будет чтение, отладка и сопровождение нашего кода.

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

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

Ревью класса LoadPaymentMethodOperation

Подозрения по поводу проблем в коде у меня возникли после чтения следующего примера:

private DataRow LoadData()
{
    if(tableName != null)
        return LoadEmployeeOperation.LoadDataFromCommand(Command);
    else
        return null;
}

Возвращение null в большинстве случаев является серьезным запахом и может приводить к неочевидным ошибкам времени исполнения. Не зря ведь Тони Хоар считает изобретение нулевых ссылок своей ошибкой на миллиард долларов, а Бертран Мейер специально ввел в языке Eiffel понятие non-nullable reference типов, чтобы четко разделить ссылочные типы, способные и не способные принимать null.

Довольно часто возвращение null скрывает ошибку в совершенно другой части программы и может говорить о невыявленных предусловиях класса. В каком случае tableName будет равен null? Нормально ли вообще, что поле tableName равняется null и какая связь между этим полем и свойством Command?

Встретив подобный код, я ожидаю проверки этого граничного условия в коде (и в тесте), но проверяет ли вызывающий метод значение на null? Не совсем:) Вызывается метод LoadData всего из одного места (что хорошо), но возвращаемое значение не проверяется:

public void Execute()
{ 
    Prepare(); 
    DataRow row = LoadData(); 
    CreatePaymentMethod(row);
}

Возвращаемое значение метода LoadRow передается другому методу. Теперь, анализируя код этого класса нужно помнить о том, что аргумент метода CreatePaymentMethod может получить null. Идем дальше.

public void CreatePaymentMethod(DataRow row)
{ 
    paymentMethodCreator(row);
}

Метод почему-то открытый, ну да ладно; все, что он делает, это вызывает делегат paymentMethodCreator, который инициализируется следующим образом:

public void Prepare()
{ 
    // methodCode инициализируется в конструкторе
    if(methodCode.Equals("hold")) 
        paymentMethodCreator = new PaymentMethodCreator(CreateHoldMethod); 
    else if(methodCode.Equals("directdeposit")) 
    { 
        tableName = "DirectDepositAccount"; 
        paymentMethodCreator = new PaymentMethodCreator(CreateDirectDepositMethod); 
    } 
    else if(methodCode.Equals("mail")) 
    { 
        tableName = "PaycheckAddress"; 
        paymentMethodCreator = new PaymentMethodCreator(CreateMailMethod); 
    }
}

Хорошо. Метод Prepare – это некая форма фабричного метода, но вместо возврата создаваемого объекта, он изменяет состояние. Данный код достаточно запутанный, но при его анализе становится ясно, что мое исходное предположение о возможной генерации NullReferenceException (NRE) при вызове метода CreatePaymentMethod(null) не подтвердился (во всяком случае в текущей реализации; насколько вам это очевидно? ;)), но проблема все равно остается.

Сложность и хрупкость текущей реализации заключается в том, что возвращаемое значение метода LoadData передается затем делегату. При расширении данного класса (ведь использование делегатов в данном случае является таким себе локальным полиморфизмом), мы должны помнить, что любой метод, который будет вызываться через делегат должен справляться с обработкой null в качестве аргумента. Такая реализация может легко привести к эффекту бабочки и неприятным исключениям, что далеко от идеала, с точки зрения критериев хорошего дизайна.

Ну и последнее. Детская ошибка все равно осталась, и находится она в методе Prepare: поле paymentMethodCreator остается неинициализированным, если methodCode невалиден (не равен "hold", "directdeposit" или "mail"). Это значит, что при передаче неверного аргумента в конструкторе объекта (methodCode инициализируется в конструкторе объекта), мы получим ошибку времени исполнения лишь при вызове метода Execute.

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

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

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

public LoadPaymentMethodOperation(Employee employee, string methodCode, 
                                  SqlConnection connection)
{
    Contract.Requires(employee != null);
    Contract.Requires(connection != null);
    Contract.Requires(IsValidMethodCode(methodCode));



     
    this.employee = employee; 
    this.connection = connection;

    
    // Метод сгенерирует исключение, если methodCode невалиден. 
    // Я переименовал метод Prepare в CreateCommandAndPaymentMethod 
    // и сделал его закрытым. 
    CreateCommandAndPaymentMethod(methodCode);
}

Аргументы методов vs состояние объекта

В другой своей книге под названием «Чистый код», Боб Мартин дает такой совет: если метод принимает 3 и более параметров, то его нужно срочно рефакторить путем протаскивания параметров через поля класса, аргументируя это тем, что такие методы сложнее понять и тестировать.

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

Многие «объектно-ориентированные» книги по кодированию проповедуют небольшие методы (вплоть до нескольких строк). В этом есть смысл, но чрезмерное увлечение этим принципом приводит к множеству методов, анализировать которые становится слишком сложно (ок, а для чего нужно поле valid? А, да оно же устанавливается в false в начале метода parse и изменяется методом ParseArgs).

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

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

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

// Вариант Боба Мартина
public void CreateDirectDepositMethod(DataRow row)
{
     string bank = row["Bank"].ToString();
     string account = row["Account"].ToString();
     method = new DirectDepositMethod(bank, account);
}
 



// Вариант без побочных эффектов
private static PaymentMethod CreateDirectDepositMethod(DataRow row)
{
     string bank = row["Bank"].ToString();
     string account = row["Account"].ToString();
     return new DirectDepositMethod(bank, account);
}

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

Хороший пример ужасных тестов

Существуют разные подходы к юнит-тестированию. Кто-то считает, что для юнит-тестов лучше всего подходит подход "белого ящика", когда тест, по сути, знает о внутренней реализации тестируемого кода и использует для проверки соответствующие детальные утверждения (assert statements).

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

Возвращаясь к коду Боба Мартина это означает, что у меня будет тест вида: Test_Unknown_Method_Code_Will_Throw_Exception или же я сделаю один параметризованный юнит-тест, который покроет все внутренние условия благодаря различным входным данным.

Одним из недостатков (ИМХО) TDD является то, что тесты начинают слишком много знать о внутренней реализации класса, что делает их более хрупкими, чем мне хотелось бы, нарушая, по сути, инкапсуляцию класса. Но иногда даже без TDD можно встретить такие тесты, на которые без содрогания смотреть невозможно. К сожалению, в книге Боба Мартина такие присутствуют:

[Test]
public void LoadingEmployeeDataCommand()
{
	operation = new LoadEmployeeOperation(123, null);
	SqlCommand command = operation.LoadEmployeeCommand;
	Assert.AreEqual("select * from Employee " +
		"where EmpId=@EmpId", command.CommandText);
	Assert.AreEqual(123, command.Parameters["@EmpId"].Value);
}

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

При этом такой тест усложняет сопрвождаемость, поскольку он дублирует код тестируемого класса, не добавляя ничего взамен. Запросы могут усложняться с течением времени, хотя с точки зрения внешнего пользователя они будут вести себя одинаково. С таким подходом мы даже спокойно не сможем изменить имя колонки или поменять запрос с “select * …” на “select id, name” без изменения соответствующего теста.

Я не говорю уже о том, что автор этого теста не разделяет юнит-тесты от интеграционных тестов. Тесты базы данных не отвечают критериям F.I.R.S.T., и должны быть отделены от остальных модульных тестов. Но в данном случае, это не самая большая проблема; самое плохое в этом случае то, что такие примеры могут серьезно повлиять на неокрепшие умы. Нужно всегда помнить, что тесты – не самоцель, а лишь средство достижения хорошего качества кода и дизайна. Подобные тесты не приближают нас к нашей цели, а скорее отдаляют от нее.

Заключение

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

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

Ну и последнее: тесты – это инструмент, который в одних руках упростит сопровождаемость, а в других – сделает ее невозможной. Тесты не должны просто дублировать код класса, а должны описывать тестируемую проблему в более абстрактной форме.

Ссылки по теме

• Что значат для вас юнит-тесты
• Критерии плохого дизайна
• Контракты vs Юнит-тесты
• Параметризованные юнит-тесты
• Пять принципов чистых тестов (F.I.R.S.T. Principles)