понедельник, 11 апреля 2011 г.

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

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

Недавно на 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) и умение бороться с ними жизненно необходимо.

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

21 комментарий:

  1. Хорошая статья, узнал про контракты. :)

    ОтветитьУдалить
  2. Добротная статья, но - ничего нового для меня.

    И ещё, "зделали" немного режет взгляд..

    ОтветитьУдалить
  3. Хорошая статья. Я как раз сейчас не что подобным занимаюсь.

    ОтветитьУдалить
  4. Спасибо, Сергей. С большим интересом читаю Ваши статьи. Много нового, полезного.

    ОтветитьУдалить
  5. Последние примечание в точку :)

    ОтветитьУдалить
  6. > А теперь мы увидим уличную магию

    Конечно увидим, ведь логика изменена. Функция теперь работает по другому. Хотя бы потому что в оригинале (buffer != null) - преобразуется в 0, а не кидает эксепшены. Наверняка нормально работающее приложение сломано. Невольно вспоминается http://avva.livejournal.com/2323823.html.

    ОтветитьУдалить
  7. @IGI: Да, я изменил поведение метода поскольку считаю, что в большинстве случаев умалчивать о багах нехорошо. А передача отрицательной длины или null-евого буфера именно багом и является.

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

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

    ОтветитьУдалить
  8. @ОП: Вы вторгаетесь в рабочий проект и вносите потенциальные ошибки. Никто не будет спрашивать вас что вы там считаете. Я просто уверен, что создание нового массива при buffer == null заложено в логику и не является багом. Менять логику не будучи уверенным в последствиях, как минимум - неправильно.

    ОтветитьУдалить
  9. @IGI: во-первых, я не в чей проект не вторгаюсь, а лишь описываю то, как я вношу исправления при рефакторинге тех проектов, в которых сам участвую на примере указанного (в тех проектах, где моим мнением интересуются). При этом я оцениваю, какой вариант исправления менее рисковый и стоит ли добавлять генерацию исключений или нет.

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

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

    З.Ы. Как вы можете быть в чем-то уверенными не будучи автором этого кода? или вы и есть его автор?

    З.Ы.Ы. Кто такой "ОП"?

    ОтветитьУдалить
  10. Мне кажется, что вы неправильно поступили с буфером, и если уж взялись за рефакторинг, то нужно было учесть все кейсы, в том числе и с buffer == null.

    Еще у вас потерялся кейс, при котором буфер не меняется при writeByteHandler == null.

    Да красиво, но не эквивалентно тому что было.

    ОтветитьУдалить
  11. Вы ставите задачу в начале статьи:

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

    Описываете ошибочный вариант решения на конкретном примере и в конце говорите:

    > я не в чей проект не вторгаюсь

    Последовательность так и блещет.

    На самом деле тут НЕТ никаких других вариантов. Единственно правильное решение в данной ситуации - оставить ЛОГИКУ как есть. Либо убрать из статьи слова о рефакторинге чужого кода в существующей системе. Или добавить в исходные данные сведения об всех вызовах функции.
    А все эти оговорки о сферических конях в каких-то других ситуациях лишь ненужный поток сознания.
    Решение поставленной задачи не-вер-но.

    > предлагаю все же не переходить на личности и вести беседу в более дружелюбной

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

    P.S.
    ОП = Орижинал Постер. Как пользоваться этой не интуитивной системой комментариев мне лень разбираться, как и копировать строку с вашим именем и обрезать лишнее. Сори.

    ОтветитьУдалить
  12. Николай, что вы имели ввиду под этим:

    > Мне кажется, что вы неправильно поступили с буфером, и если уж взялись за рефакторинг, то нужно было учесть все кейсы, в том числе и с buffer == null.

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

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

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

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

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

    ОтветитьУдалить
  13. @IGI: почему вы считаете, что чужой код является синонимом чужой системы?

    Рабочая система в большинстве случаев подразумевает исходный код ее использования. С моей точки зрения это более распространенный юз кейс, ибо когда речь заходит о повторноиспользуемых библиотеках, то там появляются совершенно другие принципы и практики; тогда вес тех или иных design decisions становится совсем другим, по сравнению с обычными системами.
    Козе понятно, что если вы не знаете всех последствий ваших изменений, то сами себе рогатые буратины. Но, повторюсь, в большинстве случаев влияние изменений можно оценить и принять взвешенное решение о том, стоит вносить breaking changes или нет.

    З.Ы. Спасибо за заботу о моем ментальном и физическом здоровье, но последний раз посоветую вам больше его не закалять.

    ОтветитьУдалить
  14. Хмм, как по мне статья отличная и показательная. Единственное в чем можно упрекнуть автора статьи, это в рефакторинге без наличия юнит тестов. Хотя возможно автор просто рассматривал программирование по контракту, как возможность замены юнит тестам? А вот автор строк исходного кода, похоже не задумывался о том, что его код возможно, придется поддерживать кому то другому.

    ОтветитьУдалить
  15. Саш, спасибо! Конечно рефакторинг без тестов (как и без анализа использования исправляемого метода) - дело очень гиблое, и по возможности нужно начинать именно с их написания, чтобы хоть как-то снизить риск того, что ты нафиг поломал этот код.

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

    ОтветитьУдалить
  16. у вас код местаим не умещается в сообщении

    вот эта строка например:
    public void WriteTransformation(ref byte[] buffer, ref int bufferLength, WriteByteHandler wri

    ОтветитьУдалить
  17. 1) слишкам многа букофф %))
    2) паентаму устал читат коменты %))
    3) но то что "пахерили" работу при условии bufer == null мне тоже сразу кинулось ф глаза %) думал написат, но тута ужо многайэ написали %))
    4) сам с таким сталкивалсо %) када думаеш шо делаиш харашо, а палучаитсо шо ламаиш то шо на саплях диржалос %)))
    5) ис ентава вывад - если менять функционалность метода - то уж делать рефакторинг и в других местах, вызывающих оны код %)) шо ест исчо куча работы парой %)) ы

    как бэ фсо %) ы

    з.ы.
    на старане тех кто за "buffer==null" ни выризат %) ы

    ОтветитьУдалить
  18. Хорошая статья.
    1.Жаль, что рефакторинг без юнит-тестов выполнен, и об этом печальном факте нигде, кроме комментов, не упомянуто.
    2. Автор прав насчет буфера и null.
    Даже если клиентский код использует передачу null (о чем довольно трудно догадаться по исходной функции), строка-утверждение
    Contract.Requires(buffer != null);
    в этом случае упадет и проблема будет легко поправлена.

    ОтветитьУдалить
  19. Сергей, вы уверенны, что, выбирать длину следует от наименьшего буфера?

    ОтветитьУдалить