*** ВНИМАНИЕ: Блог переехал на другой адрес - demin.ws ***

вторник, 6 октября 2009 г.

Закомментированные куски кода и TODO

Когда я вижу в production коде, в котором мне надо по какой-то причине разобраться, что-то типа:
...
int x = (i << 8) | (i >> 2) | ((i & 0x06) ^ 0xAA);

// if (x >= 0x12345)
// x = x >> 3;

calc_something_complicated(x);
...
мне хочется рвать и метать. Расчехлить blame, найти автора и заглянуть ему в глаза.

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

На втором месте у меня стоит отладочная печать, навеки оставленная в коде:
...
int x = (i << 8) | (i >> 2) | ((i & 0x06) ^ 0xAA);

// std::cerr << "На этот раз x = " << x << std::endl;

calc_something_complicated(x);
...
Снова получается, что автор сомневался и так и не отладил все до конца.

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

Ну на третьем месте нашего хит-парада - блоки TODO.
...
int x = (i << 8) | (i >> 2) | ((i & 0x06) ^ 0xAA);

// TODO: Заменить тип int на int64, так как на носу эра 64-разрядных машин.
// Программист Вася, 06.10.2009. Если что, вы знаете где меня искать.

calc_something_complicated(x);
...
Тут еще куда ни шло. Куски TODO можно найти автоматическим поиском при подготовке релиза и перед ответственным слиянием. Но каждый TODO должен быть подписан и датирован, а лучше еще и детально объяснен. Ни что так не помогает оценить "нужность" куска кода, как дата его последней модификации.

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

4 комментария:

  1. браво. хоть и не оригинально, но все равно.

    сам не люблю оставленные TODO. Почему их не сделать сразу, нафига оставлять на потом?

    За совет ставить дату и автора в TODO -- big thanks. Оценил.

    ОтветитьУдалить
  2. Если же этот TODO - плановая работа, баг и т.д., то этой информации место не в исходнике, а в плане/спринте/итерации и т.д. Как мне кажется - исходник должен отражать текущее состояние проекта. Его прошлое отражается в системе контроля версий и баг трекере, а будущее - в плане разработки. А TODO - это какое-то упрощенное все вместе. ;-)

    ОтветитьУдалить
  3. По поводу TODO не соглашусь. Как пример, не всегда в коде должен быть реализован весь функционал. Но оставить заглушки с комментарием и пометкой TODO для будущего релиза бывает удобно.

    ОтветитьУдалить
  4. Оставленная "на потом" отладочная печать тоже бывает полезна. Но лучше, когда она оформлена единообразно, например,

    module_debug("x=%d", x);

    и включается каким-нибудь ключиком - глобально или помодульно.

    >автор сомневался и так и не отладил все до конца

    После написания код обычно живёт своей жизнью, и бывает нужно включить отладку в конкретном месте уже после того, как автор всё отладил. Неважно, насколько он при этом "сомневался" :)

    И потом, если подходить с этой логикой, то и ассерты не нужны... Зачем они, если автор всё отладил и ни в чём уже не сомневается?

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