Критика книги Боба Мартина "Принципы, паттерны и методики гибкой разработки на языке C#"

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

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

• Контракты, состояние и юнит-тесты
  Боб Мартин известный сторонник TDD, я же предпочитаю думать о дизайне в терминах контрактов. В результате, в коде книги есть несколько багов, поскольку реализация "драйвилась" тестами, а не предусловиями и постусловиями. В этой статье также приведен пример просто ужасных тестов Мартина, за которые нужно отрывать руки по самые колени!
  Интересно, что по этой же теме, контракты vs. юнит тесты, есть интересное видео на InfoQ с участием Мартина и Коплиена: Coplien and Martin Debate TDD, CDD and Professionalism.
• Критический взгляд на принцип инверсии зависимостей
  Боб Мартин известен популяризацией принципа Инверсии Зависимостей, но мало кто обращал внимание, что при его описании используются неверные термины. В приведенной статье содержится более подробная критика паттерна, а точнее его интерпретация Бобом Мартином.
• Тестируемый дизайн vs. хороший дизайн
  В своей книге Боб Мартин показывает, как обеспечить тестируемость некоего кода путем выделения интерфейсов для всех классов, я же привожу альтернативный вариант тестируемого дизайна, который не требует обилия интерфейсов и моков.

С первых страниц книги, Мартины очень неслабо вбрасывают на вентилятор, начиная книгу следующим образом:

"Мой опыт показывает, что программисты .NET зачастую слабее тех, что пишут на Java или C++. Понятно, что бывают исключения. Однако раз за разом наблюдая за своими слушателями, я вынужден был заключить, что программисты .NET обычно хуже разбираются в методах разработки ПО, паттернах и принципах проектирования и т.п. Нередко случалось, что присутствовавшие программисты .NET вообще не слыхали об этих фундаментальных концепциях. Такое положение должно быть изменено.

Работая над этой книгой, я не раз сомневался, ставить ли свое имя на обложке книги, посвященной .NET. Я спрашивал себя, хочу ли я, чтобы меня ассоциировали с .NET и со всеми негативными представлениями, связанными с этой платформой. Но что толку отрицать? Я программист .NET. Нет! Я гибкий программист .NET. И горжусь этим."

Теперь же давайте посмотрим, чему мы можем научиться у камрадов Мартинов.

Принципы

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

Вот интересный момент:

"Гибкие команды применяют эти принципы (имеется ввиду SOLID) только, чтобы устранить запашок; когда ничем плохим не пахнет, то и принципы не применяются. Было бы ошибкой безоговорочно придерживаться некоторого принципа просто потому, что это принцип. Принципы существуют для того, чтобы помогать в устранении дурных запахов. Это не духи, которыми надо обильно поливать всю систему. Чрезмерная приверженность принципам ведет к пороку ненужной сложности."

С одной стороны, это очень прагматичная точка зрения, но с другой стороны, зачем делать столь жесткие принципы, чтобы потом вводить дополнительные правила о том, когда им следовать, а когда нет?

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

Ниже представлены описания принципов, не нарушить которые просто нельзя.

LSP

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

Или есть другой вариант: мы живем в реальном мире.

"Находит ли принцип LSP применение в реальных программах? Рассмотрим пример, взятый из проекта, над которым я работал несколько лет назад. ... В начале 1990-х ..."

Оригинал книги вышел в середине 2006-го года (уже после выхода C# 2.0 и обобщений), а тут несколько лет назад – это начало 1990-хJ

"Принцип подстановки Лисков – один из основных инструментов реализации принципа OCP".

Я бы сказал, что одним из вариантов достижения OCP является полиморфизм, а LSP говорит, что полиморфизм работает так, как мы того ожидаем. При этом есть и другие варианты достижения OCP, например, на основе функций обратного вызова.

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

DIP

Более подробно критика этого принципа описана в статье "Критический взгляд на принцип инверсии зависимостей".

Но вот одно очень эмоциональное заключение, которое очень порадовало:

"На самом деле такая инверсия зависимостей – отличный признак объектно-ориентированного дизайна. Неважно, на каком языке написана программа. Если зависимости инвертированы, значит, мы имеем объектно-ориентированный дизайн. В противном случае дизайн процедурный."

Очень спорное заявление, согласитесь!

OCP

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

Определение от Боба Мартина: программные сущности (классы, модули, функции и т. п.) должны быть открыты для расширения, но закрыты для модификации.

Таким образом у модулей есть две основные характеристики:

• Они открыты для расширения. Это означает, что поведение модуля можно расширить. Когда требования к приложению изменяются, мы добавляем в модуль новое поведение, отвечающее изменившимся требованиям. Иными словами, мы можем изменить состав функций модуля.
• Они закрыты для модификации. Расширение поведения модуля не сопряжено с изменениями в исходном или двоичном коде модуля. Двоичное исполняемое представление модуля, будь то компонуемая библиотека, DLL или EXE-файл, остается неизменным.

Определение от Бертрана Мейера: модули должны иметь возможность быть как открытыми, так и закрытыми.

При этом понятия открытости и закрытости определяются таким образом:

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

Другими словами, Мейер говорит об открытости/закрытости интерфейса модуля, я Мартин – об открытости/закрытости модуле целиком, включая его интерфейс и реализацию.

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

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

ISP

При чтении книги я неоднократно ловил себя на мысли, что книга написана не с нуля, а является именно вторым изданием, примеры которой переведены на язык C#. Причем книга переделана «в лоб», а не переработанная с использованием идиом платформы .NET.

Одним из лучших проявлений этого свойства являются примеры, приведенные при описании принципа Interface Segregation Principle.

Принцип разделения интерфейсов рассматривается на двух примерах, одним из которых является охранная система с классом Door (Дверь) и подклассом TimedDoor, которая должна выдавать звуковой сигнал, если дверь не закрыта в течение некоторого времени. Вот один из вариантов решения этой задачи, приведенный в разделе «Разделение путем делегирования», который, по словам автора, будет удовлетворять ISP:

Вот, что пишут авторы по этому поводу: «Это решение согласуется с принципом ISP и не создает связей между клиентами Door и классом Timer. … Однако оно не слишком элегантно. Каждый раз, когда мы хотим зарегистрировать тайм-аут, приходится создавать новый объект.»

Прочитав этот раздел я представил себе, что должен был сказать младший Мартин (который .NET-ый сын) после ее прочтения:

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

Тут ведь два лишних класса, а за метод DoorTimeout в классе TimedDoor, который принимает timeoutId нам вообще причиндалы оторвут! Ведь таймера в .NET есть с самого начала и «наблюдатель» в них строится на основе событий, а не на основе интерфейсов. Вот и получается, что мы можем выкинуть TimerClient и DoorTimerAdapter, а метод DoorTimeout сделать закрытым (да еще и этот timeoutId выкинуть).

Так и с точки зрения дизайна код будет чище, да и с точки зрения реализации – тоже. Так что этот пример нужно бы выкинуть, а главу переработать полностью под идиомы языка C#, а то комьюнити может и не оценить такое творчество!»

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

UML

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

Объекты не являются частью "неизменяемой логической структуры программы".

"Ассоциацией называется простое отношение, состоящее в том, что один объект храни ссылку на другой и вызывает методы другого объекта.".

Ассоциация не подразумевает сохранение ссылки. Если класс А использует Синглтон напрямую, то ссылки нет, а ассоциация есть.

"Композиция – это частный случай агрегирования. Снова отмечу, что реализация неотличима от ассоциации общего вида. На сей раз причина в том, что это отношение находит мало применений в программах на языке C#."

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

Паттерны

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

Теперь немного конкретики.

Фасад и посредник

"Оба паттерна, рассматриваемые в этой главе, преследуют одну цель: наложить какую-то политику на группу объектов. Фасад (Façade) накладывает политику сверху, а Посредник (Mediator) – снизу. Фасад виден и вводит ограничения, Посредник не виден и ни в чем не ограничивает."

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

Эти паттерны действительно не имеют ничего общего, относятся к разным категориям (фасад – структурный, посредник – поведенческий) и предназначены для решения разных зада на разных уровнях абстракции. Глава книги должна быть такой же цельной (cohesive), как и классы и модули, и попытки описать разнородные понятия в одном месте кажутся сомнительными.

Декоратор

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

WTF?! Основная суть декоратора заключается в том, что он позволяет изменить поведение без изменения интерфейса декорируемого объекта. Если же мы "пополним его методами чтения и записи", и не переопределим ни одного виртуального метода, то полученный класс не будет адекватным декоратором!

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

Посетитель

"Этот цикл зависимостей связывает все посещаемые подклассы – все типы модемов – воедино, затрудняя инкрементную компиляцию посетителей или добавление новых подклассов в посещаемую иерархию."

Очевидно, выделенный фрагмент просто не обновили после перехода с C++ на C#.

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

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

Состояние

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

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

Я всегда думал, что паттерн State на основе таблицы переходов – это одно из самых удачных решений в плане стоимости разработки/эффективности. Но, конечно же, типовое решение подразумевает использование словаря, который обеспечит О(1) сложность поиска перехода.

Шаблонный метод и Стратегия

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

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

Юнит-тесты

Более подробно об этом можно почитать в статье: "Контракты, состояние и юнит-тесты".

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

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

Во-вторых, Мартин приводит слишком большое количество тестов, написанных по принципу: выделили интерфейсы всех классов, написали тесты, СЧАСТЬЕ. Этот подход меня беспокоит тем, что поработав так пару месяцев мы получим код с хорошим покрытием, но с плохим дизайном. Именно об этом я писал в заметке "Тестируемый дизайн vs. Хороший дизайн".

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

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

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

.NET

Я уже упоминал, что при чтении книги чувствуется, что это второе издание книги, которую слегка адаптировали под язык C#, а не переработали полноценно с использованием идиом нового языка программирования. Во многих местах используются идиомы именования из Java (имена методов с маленькой буквы, КОНСТАНТЫ_ЗАПИСАНЫ_ВОТ_ТАК), добавляется метод Main в доменный объект, изредка используется понятие "пакета" вместо сборок. В примерах отсутствуют стандартные идиомы .NET: для наблюдателей используются интерфейсы и не используются события; не используются модификатор readonly, классы с ресурсами не реализуют IDisposable etc; не используются обобщения, хотя они появились за год до появления книги.

Одним из самых эпичных ляпов дан в следующей врезке "А куда делось I?".

"Вообще говоря не стоит включать в имя некую ортогональную ему концепцию, особенно если эта концепция может измениться. Например, что если мы захотим сделать ICommand не интерфейсом, а абстрактным классом? Будем отыскивать все упоминания ICommand и заменять их на Command? Будем заново компилировать и развертывать все затронутые этим изменением сборки?"

ИМХО, проще прийти в чужой монастырь со своим уставом, чем прийти в другой язык со своими идиомами. Если я программирую на Java, то я использую местные идиомы именования не зависимо от собственных предпочтений, это ПРАВИЛЬНО! Но больше всего в этой врезке мне нравится аргументация.

Младший Мартин видимо не подсказал старшему, что переход от интерфейса к абстрактному классу – это потенциальный breaking change не зависимо от того, изменим имя сущности или нет: нам в любом случае потребуется "заново компилировать и развертывать все затронутые этим изменением сборки"! Попытка подменить сборку на лету с подобным изменением приведет к краху приложения!

PrimeGenerator

Мартины очень любят состояние, поэтому при рефакторинге генератора простых чисел (стр. 80) были выделены статические поля:

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

class PrimeGenerator
{
    private static bool[] crossedOut;
    private static int[] result;
    public static int[] GeneratePrimeNumbers(int maxValue)
    {
        return null;
    }
 
    // создает и заполняет crossedOut
    private static void UncrossIntegersUpTo(int maxValue) { }
 
    // Создает result на основе crossedOut
    private static void PutUncrossedIntegersIntoResult()
    { }
}

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

Разделяемое состояние усложняет код (ведь теперь порядок вызовов важен), а также делает невозможным параллельное получение простых чисел из разных потоков, что нарушает общепринятое правило из "Framework Design Guidelines", которое гласит, что статические члены классов должны быть потокобезопасными.

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

SocketServer

Данный код рассмотрен на странице 251 и его нет в примерах, которые идут вместе с книгой. Я положил его сюда.

public interface SocketService
{
    void Serve(Socket s);
}
 
public class SocketServer
{
    private TcpListener serverSocket = null;
    private Thread serverThread = null;
    private bool running = false;
    private SocketService itsService = null;
    private ArrayList threads = new ArrayList();
 
    public SocketServer(int port, SocketService service)
    {
        itsService = service;
        IPAddress addr = IPAddress.Parse("127.0.0.1");
        serverSocket = new TcpListener(addr, port);
        serverThread = new Thread(new ThreadStart(Server));
        serverThread.Start();
    }
 
    public void Close()
    {
        running = false;
        serverThread.Interrupt();
        serverSocket.Stop();
        serverThread.Join();
        WaitForServiceThreads();
    }
 
    private void Server()
    {
        serverSocket.Start();
        running = true;
        while (running)
        {
            Socket s = serverSocket.AcceptSocket();
            StartServiceThread(s);
        }
    }


 
    // Остальное поскипано 
}

В коде есть два типа проблем: во-первых, это мелочи, типа отсутствие readonly, отсутствие интерфейса IDisposable, использование интерфейсов вместо событий для реализации набллюдателя, ручное манипулирование потоками при наличии с первой версии .NET Framework асинхронных операций и отсутствие обработки исключений.

Но самое главное, этот код является примером плохого дизайна. Ответственности классов SocketServer, ServiceRunner и наследников SocketService размыты и не понятны.

Вот, например, у нас есть класс, реализующий интерфейс SocketService. Что можно делать в методе Serve(Socket s)? Как-то он говорит, что этот метод вызывается в другом потоке? Нет. А как ему узнать, что родительский сокет закрывается и ему тоже нужно прервать выполнение своих операций? А как понять, можно ли вызывать в этом методе блокирующие методы типа Receive или же нужно крутить свой собственный бесконечный цикл?

Видно, что если тесты к коду написать нельзя, то полученный дизайн Мартинов будет ужасным. Данный код не эффективно использует встроенные возможности .NET Framework, но, самое смешное, что он просто не работает! Посмотрите на метод Close, там выставляется running в false, а затем вызывается thread.Interrupt(), который ни к чему не приведет, поскольку текущий серверный поток сейчас висит в ожидании нового подключения в методе serverSocket.AcceptSocket(). Аналогичная проблема будет и со всеми клиентскими сокетами, если они в методе Server вызовут s.Receive(): всю эту городушку нельзя будет закрыть.

TreeMap

Код целиком можно посмотреть здесь.

internal class TreeMapNode
{
    private static readonly int LESS = 0;
    private static readonly int GREATER = 1;
    private IComparable key;
    private object value;
 
    private TreeMapNode[] nodes = new TreeMapNode[2];
 
    private int SelectSubNode(IComparable key)
    {
        return (key.CompareTo(this.key) < 0) ? LESS : GREATER;
    }
}

Здесь все не так смертельно, но бросается в глаза использование java-style констант, отсутствие ключевого слова readonly для полей key и nodes, а также неудачные имена констант: LESS и GREATER, которые, на самом деле, определяют индекс поддерева.

Заключение

У меня как-то спросили, почему я оценил эту книгу на goodreads.com в 2 бала. Ну, в таком случае у меня встречный вопрос: а разве стоит ставить больше?

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

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