Как не надо писать код

По статистике мы значительно чаще читаем код, чем пишем новый. Этот код может быть своим, а может быть чужим, это может быть ревью чужого кода пред коммитом, а может быть вам дали задачу добавить новую возможность, пофиксить багу или просто разобраться в логике существующей системы. Отношение к коду, как чему-то очень важному совсем не ново, об этом написаны десятки книг и, наверное, сотни статей; но практика показывает, что обилие информации по этой теме никак не влияет на качество существующего кода в реальных системах. На практике все чаще слышишь, «вот дали бы побольше времени, вот тогда бы я все сделал как нужно, никакого г#$@о-кода бы не было» или «ладно, вот эту заплатку я делаю последний раз, а вот потом я вернусь и все исправлю».

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

Итак, вначале давайте посмотрим на тот злосчастный код, а потом уже по ходу дела решим, что с этим зверем делать:

public delegate void WriteByteHandler(byte b);
private const int PREFIX = 1;
public void WriteTransformation(ref byte[] buffer, ref int bufferLength, WriteByteHandler writeByteHandler)
{
    if (writeByteHandler != null)
    {
        bufferLength = 1 + Math.Min(Math.Max(bufferLength, 0), (buffer != null) ? buffer.Length : 0);
        byte[] newBuffer;
        if (bufferLength - 1 < ((buffer != null) ? buffer.Length : 0))
        {
            newBuffer = buffer;
        }
        else
        {
            newBuffer = new byte[bufferLength];
        }
        for (int i = bufferLength - 1; i > 0; i--)
        {
            if (newBuffer != null) if (buffer != null)
                    newBuffer[i] = buffer[i - 1];
        }
        if (newBuffer != null) newBuffer[0] = PREFIX;
        buffer = newBuffer;
    }
}

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

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

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

ПРИМЕЧАНИЕ (1)
Внимательный читатель может найти сходство описанного мной подхода с весьма популярной нынче техникой под названием «рефакторинг» и на самом деле, в этой маленькой заметке именно это и демонстрируется. Просто автор, как и большинство других программистов, зачастую сталкивается с подобным кодом на практике, но на этой самой практике разработчики, к сожалению, очень часто пользуются либо первым, либо третьим вариантом.

1. Комментарии

Первое, что бросается в глаза – это полное отсутствие комментариев в коде вообще, и комментариев для самого публичного метода в частности. Код этого метода не настолько прост, чтобы в его семантике можно было разобраться за несколько минут, имя же этого метода тоже не блещет самодокументированностью. «Капитан» в данном случае советует один из двух вариантов: либо добавить чертов комментарий, либо назвать метод по нормальному, либо сделать и то и другое (2). Безусловно, комментариев не должно быть слишком много; их должно быть ровно столько, чтобы они давали понять *назначение* кода, и не превращались в устаревшие при каждом изменении метода (3).

// Преобразование входного буффера путем добавление  байта префикса в его начало
public void WriteTransformation(ref byte[] buffer, 
    ref int bufferLength, WriteByteHandler writeByteHandler)

ПРИМЕЧАНИЕ (2)
Конечно же, именно этот конкретный пример кода мог быть изменен перед публикацией его на rsdn.ru, более того, скорее всего так оно и было. Но согласитесь, что при всем при этом на практике очень часто попадается код, который в буквальном смысле этого слова повторяет все аналогичные проблемы.

ПРИМЕЧАНИЕ (3)
Ну и последнее примечание: все добавленные мною комментарии получены самостоятельно по существующему коду, т.е. я не общался с автором исходного кода. Но, с другой стороны, на практике зачастую бывает именно так, но это не должно останавливать вас от добавления своих собственных комментариев, даже если они могут быть в чем-то ошибочными. Если вы сомневаетесь в семантике метода, то просто добавьте соответствующий комментарий, чтобы вы или другой человек, читающий этот код позднее смогли сэкономить время потраченное вами на изучение этого кода.

2. Сигнатура метода и неиспользуемые параметры

Вполне вероятно, что переменная writeByteHandler в реальном коде используется, а просто на пути к публикации фрагмента на rsdn.ru его практическая ценность исчезла, но тем не менее, неиспользуемые параметры – не редкость и в реальном коде. Если неиспользуемый параметр присутствует в публичном интерфейсе и вы не контролируете весь код использования этого метода, то на первой итерации я обычно помечаю этот метод атрибутом ObsoleteAttribute и добавляю другой метод с тем же именем, но без неиспользуемого параметра. Поскольку в данном коде я не вижу ничего кроме проверки делегата writeByteHandler на null, то рассматриваю этот параметр, как неиспользуемый и просто удаляю его из сигнатуры метода.

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

// Преобразование входного буффера путем добавление байта префикса в его начало
public byte[] WriteTransformation(byte[] buffer, int bufferLength)

3. Проверка предусловий

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

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

Итак, добавляем проверку предусловий и постусловия:

public byte[] WriteTransformation(byte[] buffer, int bufferLength)
{
    Contract.Requires(buffer != null);
    Contract.Requires(bufferLength >= 0);
    Contract.Ensures(Contract.Result<byte[]>() != null);
    // ..
}

4. Расчета размера буфера

А теперь мы увидим уличную магию, как наличие предусловий на порядок упрощают существующий код и, что называется, делают из г#$#а конфетку. Давайте еще раз взглянем на фрагмент существующего кода:

bufferLength = 1 + Math.Min(Math.Max(bufferLength, 0), 
        (buffer != null) ? buffer.Length : 0);

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

Меняем

(buffer != null) ? buffer.Length : 0 --> buffer.Length
Math.Max(bufferLength, 0) --> bufferLength

Теперь нам осталось добавить небольшой комментарий, и предыдущий малопонятный код превращается в следующий:

// Размер нового буфера должен учитывать необходимость дополнительного байта,
// который мы собираемся добавить в его начало
int newBufferLength = 1 + Math.Min(bufferLength, buffer.Length);

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

5. Создание нового буфера

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

byte[] newBuffer;
if (bufferLength - 1 < ((buffer != null) ? buffer.Length : 0))
{
    newBuffer = buffer;
}
else
{
    newBuffer = new byte[bufferLength];
}

В этом небольшом фрагменте в очередной раз проявилась страсть автора к криптографии, ну да ладно. Поскольку переменная buffer больше не может быть null, мы можем упростить условие так:

if (buffer.Length > bufferLength - 1)

Теперь мы можем заменить сравнение “>” на “>=”, что избавит нас от дополнительного вычитания:

if (buffer.Length >= bufferLength)

Ну, а теперь меняем buffer.Length и bufferLength в условии местами и заменяем “>=” на “<”, добавляем небольшой комментрий и… вуяля:

// Нам нужно удостовериться, что в исходном массиве есть место 
// под дополнительный байт префикса:
byte[] newBuffer;
if (buffer.Length < newBufferLength)
{
    newBuffer = new byte[newBufferLength];
}
else
{
    newBuffer = buffer;
}

6. Заключительный штрих

Заключительный фрагмент кода не делает ничего сверхъестественного, он всего лишь берет все элементы исходного буфера и копирует их в нових буфер сдвигая на один элемент вправо. Единственной особенностью здесь является то, что цикл должен обязательно начинаться с конца буфера, поскольку в некоторых случаях переменные newBuffer и buffer могут ссылаться на один экземпляр массива. Альтернативой циклу является использование соответсвущей перегрузки метода Array.Copy, ну а какой вариант выбрать – это дело вкуса. Так что все изменния в этом коде заключаются лишь в добавлении комментария и устранении дополнительных проверок на null.

На этом, в общем-то, изменения и заканчиваются и можно посмотреть на окончательный результат:

// Преобразование входного буффера путем добавление байта префикса в его начало
public byte[] WriteTransformation(byte[] buffer, int bufferLength)
{
    Contract.Requires(buffer != null);
    Contract.Requires(bufferLength >= 0);
    Contract.Ensures(Contract.Result<byte[]>() != null);
 
    // Размер нового буфера должен учитывать необходимость дополнительного байта, 
    // который мы собираемся добавить в его начало
    int newBufferLength = 1 + Math.Min(bufferLength, buffer.Length);
 
    // Нам нужно удостовериться, что в исходном массиве есть место под 
    // дополнительный байт префикса:
    byte[] newBuffer;
    if (buffer.Length < newBufferLength)
    {
        newBuffer = new byte[newBufferLength];
    }
    else
    {
        newBuffer = buffer;
    }
 
    // Теперь нам нужно скопировать все элементы из массива buffer в newBuffer, 
    // сместив каждый из них на  один индекс вправо, чтобы в нулевой индекс
    // поместить PREFIX. Начинаем перебор элементов с конца, поскольку 
    // newBuffer и buffer могут ссылаться на один и тот же экземпляр массива
 
    // Вместо цикла можно воспользоваться следующим методом:
    // Array.Copy(buffer, 0, newBuffer, 1, newBufferLength);
    // Какой вариант более читательный и предпочтительный - дело вкуса
    for (int i = newBufferLength - 1; i > 0; i--)
    {
        newBuffer[i] = buffer[i - 1];
    }
 
    // Добавляем префикс в начало нового массива
    newBuffer[0] = PREFIX;
 
    return newBuffer;
}

Как по мне, результат на лицо.

Заключение

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

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

ПРИМЕЧАНИЕ
Безусловно, что читатели этого блога не способны на такое; подобный код является делом рук злостных говнокодеров, которые приходят в проект, делают свое страшное дело и уходят. Конечно же, бывают и такие случаи, когда в заголовке файла почему-то стоит твоя собственная фамилия, но это лишь говорит о временнных помутнениях сознания, ведь мы-то с вами знаем, что в здравом уме мы на это не способны.