НЕ пишите комментарии!

Заголовок этого поста может показаться странным. Разве нам не советуют книжки и учебники писать комментарии и документировать, что делает код? "Пишите комментарии, а то через месяц и сами не вспомните, что делает этот код" - звучит знакомо?

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

Типичные комментарии в программе

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

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

При этом рекомендуется:

• помещать комментарий недалеко от начала модуля для пояснения его назначения;
• помещать комментарий перед объявлением класса;
• помещать комментарий перед объявлением метода;
• избегать очевидных комментариев:
  

  i := i + 1; // добавить к i единицу

  или (сама Delphi тут не безгрешна):
  

  type
    TForm1 = class(TForm)
    private
      { Private declarations }
    public
      { Public declarations }
    end;

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

Мысль кратко

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

Распространённый миф: код пишется для того, чтобы его читала (выполняла) машина. Это в корне неверно. Код пишется для того, чтобы его читали люди. Именно поэтому более важно, чтобы код был читабелен и прост в понимании, чем эффективен. Эффективный код, который невозможно проверить на корректность, имеет весьма малую ценность по сравнению с гибким и очевидным кодом, который выполняется несколько медленнее. Да, это пересекается с предыдущей темой.

(регулярные выражения - пример write-only языка; единожды написав регулярное выражение, вы обязательно снабдите его комментарием - ведь вам через месяц или другому человеку будет совершенно не ясно, что же оно делает; кроме того, нет никакой возможности сходу проверить его корректность при чтении кода: вам придётся остановиться и вдумчиво анализировать его корректность; ах, да, и, конечно же, интерпретация этого выражения будет зависеть от движка - have a nice day!)

Примеры

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

Пример 1 - пояснение непонятного кода

Давайте вы попробуете угадать, что делает эта функция:

function DoSomething(const A: Extended): Extended;
begin
  Result := 6.283185307179 * A;
end;

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

Ну, раз функция непонятна - для этого у нас есть комментарии, разве нет?

// Вычисление длины окружности
function DoSomething(const A: Extended { радиус } ): Extended;
begin
  Result := 6.283185307179 * A; // 2 * Пи * радиус
end;

Во! Совсем другое дело! Теперь стало понятно, что делает эта функция. Но зачем нам комментарии?

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

function CalcCircleLength(const ARadius: Extended): Extended;
const
  Pi = 3.141592653589;
begin
  Result := 2 * Pi * ARadius;
end;

Разве при этом функция не стала ещё более очевидной, чем с использованием комментариев?

Пример 2 - разделители

Вот пример большой и сложной функции (окей, это не самая большая функция - я экономлю место и ваше время, но её размера достаточно для демонстрации):

procedure TClientSocket.Open;
var
  Blocking: Longint;
begin
  if FConnected then
    Exit;
  InternalStartup;
  FSocket := socket(PF_INET, FType, IPPROTO_IP);
  if FSocket = INVALID_SOCKET then
    Error(SysErrorMessage(WSAGetLastError));
  FAddr.sin_family := PF_INET;
  FAddr.sin_addr := LookupName(FHost);
  FAddr.sin_port := htons(FPort);
  WSAAsyncSelect(FSocket, 0, 0, 0);
  Blocking := 0;
  ioctlsocket(FSocket, FIONBIO, Blocking);
  if connect(FSocket, FAddr, SizeOf(FAddr)) <> 0 then
    Error(SysErrorMessage(WSAGetLastError));
  FConnected := FSocket <> INVALID_SOCKET;
end;

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

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

procedure TClientSocket.Open;
var
  Blocking: Longint;
begin
  if FConnected then
    Exit;

  InternalStartup;

  // Инициализация сокета
  FSocket := socket(PF_INET, FType, IPPROTO_IP);
  if FSocket = INVALID_SOCKET then
    Error(SysErrorMessage(WSAGetLastError));

  FAddr.sin_family := PF_INET;
  FAddr.sin_addr := LookupName(FHost);
  FAddr.sin_port := htons(FPort);
  WSAAsyncSelect(FSocket, 0, 0, 0);

  // Настройка
  Blocking := 0;
  ioctlsocket(FSocket, FIONBIO, Blocking);

  // Собственно подключение
  if connect(FSocket, FAddr, SizeOf(FAddr)) <> 0 then
    Error(SysErrorMessage(WSAGetLastError));
  FConnected := FSocket <> INVALID_SOCKET;
end;

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

procedure TClientSocket.Open;

  procedure SocketOpen;
  begin
    FSocket := socket(PF_INET, FType, IPPROTO_IP);
    if FSocket = INVALID_SOCKET then
      Error(SysErrorMessage(WSAGetLastError));
  end;

  procedure SocketSelect;
  begin
    FAddr.sin_family := PF_INET;
    FAddr.sin_addr := LookupName(FHost);
    FAddr.sin_port := htons(FPort);
    WSAAsyncSelect(FSocket, 0, 0, 0);
  end;

  procedure SocketSetup;
  var
    Blocking: Longint;
  begin
    Blocking := 0;
    ioctlsocket(FSocket, FIONBIO, Blocking);
  end;

  procedure SocketConnect;
  begin
    if connect(FSocket, FAddr, SizeOf(FAddr)) <> 0 then
      Error(SysErrorMessage(WSAGetLastError));
    FConnected := FSocket <> INVALID_SOCKET;
  end;

begin
  if FConnected then
    Exit;

  InternalStartup;

  SocketOpen;
  SocketSelect;
  SocketSetup;
  SocketConnect;
end;

Насколько очевиднее стал код - и никаких комментариев. Заметьте, как каждая подпрограмма делает всего одну задачу.

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

Разбиение на более мелкие блоки вместо оформления комментированием - достаточно полезный инструмент, хотя по началу он и кажется "слишком громоздким": "ну посмотрите же, как он плодит мелкие функции!". Конечно, злоупотреблять этим не стоит, но часто это ложное ощущение. Попробуйте повыделять фрагменты кода в подпрограммы - и посмотрите как ваш код станет прозрачнее для понимания! Надо также заметить, что это общий принцип, применимый не только к подпрограммам, но и к объектам (вы можете разбивать один объект на несколько), модулям (разнесение огромного модуля на несколько мелких) или даже исполняемым модулям (разбить большую DLL на несколько).

Пример 3 - условия и циклы

Поехали дальше:

begin
  ...
  if (Pointer(ClassType) <> nil) and
     (IsValidBlockAddr(Pointer(ClassType) + vmtSelfPtr, (vmtCreateObject - vmtSelfPtr))) and
     (TClass((Pointer(ClassType) + vmtSelfPtr)^) = ClassType) and
     (IsValidBlockAddr(Pointer(ClassType) + vmtClassName, SizeOf(Pointer))) and
     (IsValidBlockAddr(PPAddress(Pointer(ClassType) + vmtClassName)^, 1)) and
     (IsValidBlockAddr(PPAddress(Pointer(ClassType) + vmtClassName)^ + 1, PByte(Pointer(ClassType) + vmtClassName)^)) and
     (IsValidSymbolName(ClassType.ClassName)) then
  begin
    ...
  end;
  ...
end;

Угх... увидеть такое при чтении кода равносильно удару под дых. Единственное, что можно сказать про этот код - он что-то делает с классом. Комментарии спешат на помощь:

begin
  ...
  // Является ли ClassType допустимым классом?
  if
     // Допустимый указатель...
     (Pointer(ClassType) <> nil) and
     // Читабельные данные класса...
     (IsValidBlockAddr(Pointer(ClassType) + vmtSelfPtr, (vmtCreateObject - vmtSelfPtr))) and
     // Корректное поле Self...
     (TClass((Pointer(ClassType) + vmtSelfPtr)^) = ClassType) and
     // Читабельный указатель ClassName...
     (IsValidBlockAddr(Pointer(ClassType) + vmtClassName, SizeOf(Pointer))) and
     // Читабельный размер ClassName...
     (IsValidBlockAddr(PPAddress(Pointer(ClassType) + vmtClassName)^, 1)) and
     // Читабельное значение ClassName...
     (IsValidBlockAddr(PPAddress(Pointer(ClassType) + vmtClassName)^ + 1, PByte(Pointer(ClassType) + vmtClassName)^)) and
     // Корректное значение ClassName...
     (IsValidSymbolName(ClassType.ClassName)) then
  begin
    ...
  end;
  ...
end;

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

...

  function IsValidClass(const ClassType: TClass): Boolean;
  var
    ClassPntr: PAddress;
  begin
    ClassPntr := Pointer(ClassType);
    Result :=
      // Допустимый указатель...
      (ClassPntr <> nil) and
      // Читабельные данные класса...
      (IsValidBlockAddr(ClassPntr + vmtSelfPtr, (vmtCreateObject - vmtSelfPtr))) and
      // Корректное поле Self...
      (TClass((ClassPntr + vmtSelfPtr)^) = ClassType) and
      // Читабельный указатель ClassName...
      (IsValidBlockAddr(ClassPntr + vmtClassName, SizeOf(Pointer))) and
      // Читабельный размер ClassName...
      (IsValidBlockAddr(PPAddress(ClassPntr + vmtClassName)^, 1)) and
      // Читабельное значение ClassName...
      (IsValidBlockAddr(PPAddress(ClassPntr + vmtClassName)^ + 1, PByte(ClassPntr + vmtClassName)^)) and
      // Корректное значение ClassName...
      (IsValidSymbolName(ClassType.ClassName));
  end;

begin
  ...
  if IsValidClass(ClassType) then
  begin
    ...
  end;
  ...
end;

(Замечу, что саму функцию IsValidClass тоже можно попробовать улучшить, но это выходит за рамки примера)

Насколько проще стало условие! Всего одна строка вместо почти десятка (и даже больше, если это комментировать). Код упростился и его стало проще проверять на корректность: вы можете сначала проверить корректность основной функции, а потом проверить корректность под-функции IsValidClass.

Отсюда следует простое правило: если в if, for, while или until у вас стоит нечто большее, чем идентификатор (как вариант - вызов функции) - рассмотрите возможность выделения этого кода в отдельную функцию.

Пример 4 - комментирование использования

Ещё одно использование комментариев - пояснение, как должна использоваться подпрограмма/класс. Вот один из вариантов:

// Должна вызываться только после того, как все параметры были вычислены
function TFormatter.FormatLine(...): String;
begin
  ...
end;

Не кажется ли вам, что такой вариант будет лучше?

function TFormatter.FormatLine(...): String;
begin
  Assert(Calculated);
  ...
end;

Во-первых, код описывает сам себя: функция может вызываться только когда свойство Calculated = True. Во-вторых, этот вариант имеет и другое преимущество: указанное ограничение не только описано на словах, но и насильно выполняется (принцип "если вы не хотите делать правильно - вас заставят это делать" - вообще очень правильный). Если вы ошибочно вызовете этот метод до вычисления параметров форматирования, то вместо неверных результатов вы получите ошибку (исключение).

(Я сильно завидую в этом плане Microsoft-ской студии, новые версии которой умеют форсировать достаточно сложные условия (типа "параметр три указывает размер буфера в параметре два") и просто не позволит вам вызвать метод с неверными параметрами)

Документация в коде

Ну, комментарии также используются для более подробного комментирования кода (когда в комментариях приводятся инструкции и даже философия архитектуры кода) и даже ведения документации, например:

{*------------------------------------------------------------------------------
  Конвертирует дату-время из отчёта в TDateTime.

  @param  AValue Строка, представляющая дату из отчёта. Имеет формат вида 'Sun, 25 Jan 2009 12:48:15 +0300'.
  @return Значение TDateTime (по местному времени), которое соответствует параметру AValue, или 0, если AValue не содержит корректного представления даты.
  @throws нет
  @see    GetDate
-------------------------------------------------------------------------------}
function GetDateTime(const AValue: String): TDateTime;
begin
  ...
end;

Последнее, при использовании специального оформления, позволяет автоматически генерировать файл справки, содержащий описания подпрограмм кода.

Хорошо это или плохо - момент достаточно спорный, и каждый решает его для себя сам.

Лично моё мнение таково: таким комментариям в коде не место. Зачастую у вас получается описание больше, чем сам код. Это раздувает исходники, и иногда их становится сложнее читать, потому что логика и смысловая нагрузка кода теряются в комментариях.

Примечание: я поменял своё мнение относительно этого вопроса - см. этот пост.

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

Но ведь комментарии зачем-то нужны?

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

Вот пример нужного комментария:

procedure NameThreadForDebugging(const ATID: Cardinal; const AThreadName: AnsiString);
type
  TThreadNameInfo = record
    InfoType: LongWord;     
    Name: PAnsiChar;    
    ThreadID: LongWord; 
    Reserved: LongWord;    
  end;
const
  cSetThreadNameExcep = $406D1388;
  DefaultInfoType     = $1000;
var
  ThreadNameInfo: TThreadNameInfo;
begin
  if IsDebuggerPresent then
  begin
    // См. "Setting a Thread Name (Unmanaged)":
    // http://msdn.microsoft.com/en-us/library/xcb2z8hs(VS.71).aspx

    FillChar(ThreadNameInfo, SizeOf(ThreadNameInfo), 0);

    ThreadNameInfo.InfoType := DefaultInfoType;
    ThreadNameInfo.Name := PAnsiChar(AThreadName);
    ThreadNameInfo.ThreadID := ATID;

    try
      RaiseException(cSetThreadNameExcep, 0, SizeOf(ThreadNameInfo) div SizeOf(LongWord), @ThreadNameInfo);
    except
    end;
  end;
end;

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

Другой пример - пометки о исправлениях ошибок или предотвращении их появления. К примеру, исправили вы ошибку - пометили исправление:

// было:

ReadKey(HKEY_LOCAL_MACHINE, ...);

// Стало:

// http://localhost/view.php?id=85
ReadKey(HKEY_CURRENT_USER, ...);

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

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

На самом деле...

А на самом деле, всё вышеизложенное - не более чем один из видов рефакторинга: улучшения существующего кода. Для продолжения ознакомления с этой темой я рекомендую книгу Мартина Фаулера "Рефакторинг. Улучшение существующего кода".

См. также:

• Красота заключается в простоте.
• Кодируйте на языке предметной области.
• Код - это дизайн.
• Разметка кода имеет значение.