понедельник, сентября 15, 2008

Правила хорошего кода

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

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

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

Итак, правила хорошего кода:

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

  2. Повсеместно избегайте литералов. Заменяйте их константами. Числовые литералы (известные, как магические числа) затрудняют понимание кода. Строковые литералы создают проблемы при локализации / глобализации. Любые повторяющиеся значения литералов создают проблемы при изменении кода. Избегайте констант там, где можно использовать перечислимые типы.

  3. Пишите короткие методы (функции). Есть различные мнения по поводу оптимального размера процедур и функций. Я использую следующий критерий: если весь метод нельзя увидеть целиком на экране или странице – это слишком большой метод. С увеличением размера функции комбинаторная сложность управляющих структур (условий циклов и т.д.) и переменных возрастает нелинейно, и очень быстро начинает создавать проблемы не только в обеспечении надежности кода, но и в его понимании. Зачем вам лишние проблемы? Разделяйте большие функции на более мелкие. Первые кандидаты на выделение в отдельные функции, это сложные условия в предложениях if и тела циклов. Выделяйте в функции отдельные функциональные блоки. Если функция вычисляет и сохраняет, то выделите функцию, которая вычисляет, и функцию, которая сохраняет.

  4. Избегайте любого дублирования кода (правило трех О «Once and Only Once»). Существует масса приемов рефакторинга, которые помогают выполнять это правило. Используйте константы вместо литералов, выделяйте повторяющийся код в отдельные методы.

  5. Избегайте глобальных переменных (переменных с глобальной областью видимости). Глобальные константы - это хорошо, глобальные функции - это нормально, глобальные переменные – это зло. А как же быть с настройками приложения, спросите вы? Большинство платформ предлагают для этого специальные средства. Если же таких средств нет, используйте глобальные функции вместо глобальных переменных. Вместо Boolean MyGlobalFlag используйте функцию Boolean GetMyGlobalFlag(), которая будет возвращать копию текущего значения вашего глобального флага, а само его значение спрячьте за функциями доступа.

  6. Контролируйте доступ. Модификатор public должны иметь только те члены класса, которые составляют публичный контракт этого класса и которые действительно используются другими классами. Все остальные члены должны быть скрыты для внешнего доступа (инкапсулированы). Следите за публичным контрактом своих классов и не допускайте появления в них ничего лишнего. Например:


    public class Action
    {
    public void DoAction(Context context)
    {
    string message = string.Empty;
    If(IsInternal(context))
    message += PrepareInternalMessage(context);
    else
    message += PrepareExternalMessage(context);
    SendMessage(message);
    }
    private string PrepareInternalMessage(Context context){…}
    private string PrepareExternalMessage(Context context){…}
    private bool IsInternal(Context context) {…}
    // этот метод тоже должен быть private
    public void SendMessage(string message){…}
    }


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

  7. Не совмещайте в одном классе разные обязанности (принцип Single Responsibility Principle). Вот, пример базового класса для реализации действий:


    public class ActionBase
    {
    public abstract void DoAction(Context context);
    // полезная фича - запись в лог
    public void WriteLog(string message){…}
    }


    Здесь программист решил, что функция записи в лог потребуется во всех реализациях классов – действий и поэтому полезно будет ее вынести в базовый класс. Но основное назначение наследников ActionBase это выполнение действий (DoAction), а запись в лог это совсем другая обязанность. Ну и что с того? Вероятно, что запись в лог потребуется не только для классов-действий, но и для многих других классов. И нам придется либо создавать экземпляр класса-действия для записи в лог (т.е. использовать класс не по назначению), либо продублировать метод WriteLog в других классах. Оба варианта неприемлемы. Поэтому метод WriteLog следует вынести в отдельный класс, который будет заниматься исключительно записью в лог.

  8. Не разговаривай с незнакомцами (правило Деметры). Внутри метода следует обращаться только:
    • К параметрам, переданным в метод (к свойствам и методам этих параметров, если это объекты)

    • К членам объекта, которому принадлежит метод (this.xxx)

    • К объектам, созданным в данном методе.


    Цель - избежать связывания с непрямыми объектами.

  9. При построении иерархий наследования старайтесь не изменять и не расширять публичный контракт базового класса в наследниках. Данное правило напрямую вытекает из известного принципа подстановки Лискоу (LSP - Liskov Substitution Principle). Вот пример иерархии, который надо избегать:


    class Shape
    {
    public int X;
    public int Y;
    }
    class Rectangle : Shape
    {
    public int Width;
    public int Height;
    }
    class Circle : Shape
    {
    public int Radius;
    }


    Придерживаясь данного правила, вы защищаете уже написанный код от изменений в классах наследниках. Крайне неприятна ситуация, когда в различные места существующего кода приходится вносить многочисленные изменения при появлении нового класса наследника. Обычно такой код насыщен операторами if и case, выполняющими обращения к специфичным членам классов наследников.
    И все же. Наследники часто расширяют контракт базовых классов, достаточно взглянуть на классы .Net Framework. Почему? Такой подход часто оправдан при проектировании библиотек и фрэймворков. В этом случае базовый класс служит не столько для определения базового интерфейса, сколько для размещения логики взаимодействия с механизмами фрэймворка. Пример такой иерархии, компоненты .Net WinForms.

  10. Разделяйте команды и запросы (принцип QCS - Query Command Separation). Каждый метод должен представлять собой либо команду либо запрос. Метод-команда выполняет какие либо действия, и, возможно, изменяет состояние объекта. Метод-запрос возвращает данные объекта, но не меняет его состояние. Следование данному принципу вы всегда можете быть уверены, что состояние объекта остается неизменным, когда вы используете его данные, и всегда изменяется явно, при вызове методов-команд (к сожалению, возможности большинства языков не позволяет явно указать компилятору, что в методе не допустимо изменение состояния объекта).
    Правильно:


    class Counter
    {
    private int _value;
    public int GetCurrentValue(){ return _value}
    public void Increment(int delta) { _value += delta;}
    public void Decrement(int delta) { _value -= delta;}
    }


    Неправильно:


    class Counter
    {
    private int _value;
    public int GetCurrentValue(){ return _value}
    public int Increment(int delta) { _value += delta; return _value;}
    public int Decrement(int delta) { _value -= delta; return _value;}
    }


25 комментариев:

Igor комментирует...

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

И вообще, мир спасут интерфейсы и позднее связывание :)

Livce комментирует...

Ещё будет полезно почитать http://www.refactoring.com.
Проблема "smell" кода будет всегда возникать в смешанных командах, особенно если дело касается legacy кода. И ничего с этим не поделаешь, если члены команды не принимают совместные правила игры. Скорее всего, решение кроется в самообразовании.
А за статью спасибо. Как всегда краткая, понятная и лаконичная.

Som aka Barabaka комментирует...

А с каких пор LSP накладывает ограничения на расширения интерфейса базового класса? Определение от дядьки Боба Мартина - Subtypes must be substitutable for their base types. То есть, если рассматривать пример с Shape, Rectangle и Circle, то ошибка не в расширении интерфейса Shape, а в том (предположительно, по приведенному коду явно не видно), что координаты x,y из базового класса определяют центр такой фигуры как круг и верхний правый угол для прямоугольника. Результат - коду, который обращается к Shape нужно будет знать, как правильно вести себя с этими x и y. Характерный smell нарушения LSP - код вида

public void Overlap (Shape shape)
{
if (shape is Rectangle)
{
// перекрываем, используя правый верхний угол
}
else
{
// перекрываем по центральной координате
}
}

Анонимный комментирует...

>>Глобальные константы - это хорошо

ИМХО это зло, по крайней мере публичные. Изменения занчения константы не будет видно в других сборках без перекомпиляции. Приватные или интернал - намана. Если нужны публичные то тока ридонли статические поля.

Sergey Rozovik комментирует...

to Mike Chaliy
>Изменения занчения константы не будет видно в других сборках без перекомпиляции.

Это более менее актуально для библиотек. Для простых проектов такая проблема не особо остра, все перекомпилируется целиком.

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

Sergey Rozovik комментирует...

to somaka
Smell в этом примере во первых в том что X и Y трактуются наследниками по разному. Во-вторых в том что мы не можем задать координаты фигур используя только интерфейс базового класса, и нам для этого всякий раз нужно использовать dynamic cast. А происходит это из-за неудачного расширения интерфейса базового класса.

Som aka Barabaka комментирует...

to sergey rozovik

Не совсем понятно, что имеется в виду, когда речь идет о невозможности задать координаты через интерфейс базового класса - выставляйте x и y, координаты поменяются. Для чего dynamic cast? Может, на примере будет понятнее что именно имелось в виду?

Sergey Rozovik комментирует...

>Не совсем понятно, что имеется в виду, когда речь идет о невозможности задать координаты через интерфейс базового класса - выставляйте x и y, координаты поменяются.

X и Y это только часть координат. Вторая часть объявлена в интерфейсе унаследованных классов, причес к каждом классе своя. Это я и имел в виду.

Som aka Barabaka комментирует...

<< X и Y это только часть координат. Вторая часть объявлена в интерфейсе унаследованных классов, причес к каждом классе своя. Это я и имел в виду. >>

То есть имеется в виду, что нужно добавить радиус в прямоугольник или как? o_O

Конечно, есть различия в интерфейсе, как и в сущности круга и прямоугольника. Это, в общем-то вполне естественно и никакой мегапринцип проектирования этого попрать не имеет права.

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

public class Rectangle
{
public virtual int Width { get; set; }

public virtual int Height { get; set; }

public int GetArea()
{
return Height * Width;
}
}

public class Square: Rectangle
{
public override int Width
{
get { return base.Width; }
set
{
base.Width = value;
base.Height = value;
}
}

public override int Height
{
get { return base.Height; }
set
{
base.Width = value;
base.Height = value;
}
}
}

Казалось бы, логично - чем квадрат не специфическая версия прямоугольника? Вспоминаем вормулировку дядьки Боба - Subtypes must be substitutable for their base types. Пробуем.

[TestFixture]
public class LspDemoTests
{
[Test]
public void AreaOfRectangle()
{
Rectangle r = new Rectangle ();

r.Width = 2;
r.Height = 3;

Assert.IsEqual(r.GetArea(), 6);
}

[Test]
public void AreaOfRectangle()
{
Rectangle r = new Square();

r.Width = 2;
r.Height = 3;

// Тут мы ведь не знаем, что у нас квадрат, так ведь?
// Тест не проходит
Assert.IsEqual(r.GetArea(), 6);
}
}

Сущность нарушения в том, что вызывающий код _обязан_ знать с кем имеет дело, с квадратом или прямоугольником, иначе поведение станет неожиданным - как в примере выше, где один и тот же код работает для прямоугольника и падает для квадрата. Здравствуй, оператор is по количеству наследников.

Практический вывод вовсе не в том, что нельзя расширять интерфейс объектов - уж поверьте на слово, можно и нужно, гораздо лучше добавления радиуса в прямоугольник :-). А вот перед тем как наследоваться от неабстрактного класса подумать надо раз 10, и выделить таки абстрактный класс.

Sergey Rozovik комментирует...

>То есть имеется в виду, что нужно добавить радиус в прямоугольник или как?

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

Som aka Barabaka комментирует...

> Обсуждаемая иерархия классов приведена в качестве неудачного примера.

А как вы себе представляете ее приведение в более удачный вид?

Sergey Rozovik комментирует...

> А как вы себе представляете ее приведение в более удачный вид?

Ну, например, поскольку наследники по разному трактуют значение X и Y, их можно переместить из базового класса в наследники.

А вообще, не очень интересно обсуждать абстрактные способы улучшения абстрактной иерархии :)

vilner комментирует...

Сергей, не прокомментируешь правило -"Не разговаривай с незнакомцами"?
Если в методе класса происходит обращение к объекту-синглтону, который не является членом класса, является ли это нарушением правила?

Sergey Rozovik комментирует...

to vilner
То, что синглтон позволяет легко и непренужденно нарушать правило Деметры, как раз и записывают в длинный перечень его недостатков. Получается, что мы не можем узнать из контракта класса, о том что он использует синглтон.
С другой строны - синглтон это практически в чистом виде global variable, что тоже не есть гуд.
Синглтон практически всегда можно заменить фабрикой или сервис-провайдером, и они не будут нарушать правило Деметры.

vilner комментирует...

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

vilner комментирует...

Еще, если несложно, напиши твое отношение к двухступенчатой инициализации объектов.

Sergey Rozovik комментирует...

"Двухступенчатая инициализация объектов" это что ты имеешь в виду?

vilner комментирует...

Имею ввиду такие конструкции, где в конструкторе происходит частичная инициализация членов класса, либо полная инициализация невалидными значениями (т.е. в общем виде объект не работоспособен), а окончательная инициализация происходит в отдельной функции.
public class A
{
private int m_i;

public A()
{
i = 1;
}

public bool init(int i)
{
m_i = i;
return true;
}
.....

}

Sergey Rozovik комментирует...

to vilner
Понятно.
Частичная инициализация объектов - это конечно плохо. Обычно это говорит о проблемах в проектировании жизненного цикла объектов, и о проблемах в зависимостях от других классов (часто неявных). Надо стараться перепроектировать дизайн, чтобы избегать таких ситуаций. Причем препроектирование обычно затрагивает другие классы.
Но не всегда это возможно осуществить, иходныйй код может быть недоступен или вы используете хитрый фрэймворк, который диктует свои правила игры.
Что тогда делать? Попытаться перепроектировать внутреннее устройство класса, чтобы он был валиден во всех своих состояниях. Паттерн State тут вам в помошь.

Idsa комментирует...

Хотел бы уточнить по поводу синглтона и нарушения закона Деметра. Согласно формальному описанию этого закона в Википедии, любое статическое свойство само по себе нарушает закон Деметра. Так ли это? Хотя в любом случае синглтон усугубляет ситуацию (добавляя зависимость вида Class.Instance.Method).

Begemot комментирует...

А чем плох последний пример, когда мы возвращаем новое значение после инкремента?
Возврат значения ведь никак не лишает нас уверености в изменении данных при вызове методов-команд ?

Sergey Rozovik комментирует...

to Begemot
Соглашение Query Command Separation реально бывает полезным в больших проектах. Методы- действия возвращающие значения обычно всегда семантически неоднозначны. Допустим есть класс Вертолет и метод-действие Взлелеть. Что должен вернуть Вертолет.Взлететь()? Boolean (в том смысле, что взлетел таки)? Double (в том смысле. как высоко взлетел)?

Begemot комментирует...

Я бы вернул Double так как он дает ответы на оба вопроса сразу:)
Но вопрос же не в этом, вопрос возвращать что-то или не возвращать ничего.
В данном случае я вижу дополнительную полезность в возврате значения (компактность \ читаемость вызывающего кода), и не вижу пробем этом. Поэтому и спрашиваю почему это плохо.

А семантическую неоднозначначность решает документация\комментарии:)

Sergey Rozovik комментирует...

to Begemot
Это плохо тем, что нарушает хорошее правило: методы которые возвращают значения - не меняют внутреннего состояния объекта (только не спрашивай меня почему оно хорошее).

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

P.S. В общем, однозначно - читать Code Complete :)

Begemot комментирует...

Тяжело так с ходу поверить что методы сигнализирующие об успешности\неуспешности себя возвратом boolean это плохая идея. Значит буду читать :) Потом попробую переосмыслить еще раз.