Уменьшаем количество ошибок с помощью чек-листа Code Review

[evgenx]

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

[Mrrl]

•Все ли входные данные проверяются (на корректный тип, длину, формат, диапазон) и декодируются?

А что такое «декодируются» в данном контексте?

[vovochkin]

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

[Mrrl]

Это было не замечание, а вопрос :) Думаю, что входные данные надо декодировать, а выходные — кодировать. Но боюсь, что этот пункт говорит о чём-то более глубоком — вроде того, что данные должны преобразовываться в канонический для программы формат в определённых точках программы, а не блуждать по ней во всех возможных форматах одновременно. Скорее всего, я ошибаюсь, поэтому и хотел прояснить.

[ov7a]

Из статьи про эффективные ревью:

Вам не надо спорить о форматирование и кодстайле. Есть множество инструментов, которые последовательно освещают эти вопросы. Важно, чтобы код был корректным, понятным и поддерживаемым. Конечно, стиль и форматирование — часть этого, но вы должны дать инструментам проверить эти вещи.

В чек-листе:

Соответствует ли код вашему стилю написания кода? Обычно это относится к расположению скобок, названиям переменных и функций, длинам строк, отступам, форматированию и комментариям.

Сами себе противоречат?

[Lopinopulos]

Я полагаю, что в качествен инструментов имелись ввиду две вещи: стандартные соглашения форматирования и автоформатирование в IDE.

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

[vovochkin]

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

[Lopinopulos]

Полностью согласен, но на деле не все команды такие вещи используют.

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

Та же проверка входных данных на тип годится только для языков с нестрогой типизацией :)

[vovochkin]

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

[xni]

Идея хорошая, и я о ней узнал из книги www.amazon.co.uk/Peer-Reviews-Software-Addison-Wesley-Information/dp/0201734850/ref=sr_1_1?ie=UTF8&qid=1423907706&sr=8-1&keywords=software+review могу дополнить своими (С++)

— все ли деструкторы методов с виртуальными функциями помечены как virtual? (да, это нужно не всегда и порой я вижу ситуации когда в этом нет необходимости)
— хорошо ли я понимаю кто владеет сырыми указателями и понимаю когда он объект будет удален
— хорошо ли я понимаю какая функция на каком потоке будет вызвана и что из этого следует
— есть ли проверки на разыменование указателей, могут ли указатели быть нулевыми?
— нет ли проблем с кодировками при переходе от string к wstring

[Comedian]

del

[artspb]

А куда делась статья про эффективность код ревью? И почему?

[vovochkin]

Там не очень хорошее качество перевода. В комментариях справедливо указали на это, и я спрятал статью в черновики, чтобы отредактировать ее когда-нибудь. Это «когда-нибудь» так и не наступило, к сожалению — так что я вытащил статью из черновика в том же состоянии, как и спрятал ее туда.