Комментарии 13
Спасибо большое. Буду чекать код. Сам пытался составить подобный чеклист, но он все время получался не достаточно полным.
•Все ли входные данные проверяются (на корректный тип, длину, формат, диапазон) и декодируются?
А что такое «декодируются» в данном контексте?
Спасибо, я поправил перевод. В оригинале вообще про выходные данные говорится. Здесь речь может идти о том, что данные могут быть получены неизвестно откуда, и их нельзя отдавать в оригинальном виде. Пример: защита от XSS в случае веб-разработки. Но я от этой темы далек, так что буду рад, если кто-то сможет уточнить понятнее и точнее.
Это было не замечание, а вопрос :) Думаю, что входные данные надо декодировать, а выходные — кодировать. Но боюсь, что этот пункт говорит о чём-то более глубоком — вроде того, что данные должны преобразовываться в канонический для программы формат в определённых точках программы, а не блуждать по ней во всех возможных форматах одновременно. Скорее всего, я ошибаюсь, поэтому и хотел прояснить.
Из статьи про эффективные ревью:
В чек-листе:
Сами себе противоречат?
Вам не надо спорить о форматирование и кодстайле. Есть множество инструментов, которые последовательно освещают эти вопросы. Важно, чтобы код был корректным, понятным и поддерживаемым. Конечно, стиль и форматирование — часть этого, но вы должны дать инструментам проверить эти вещи.
В чек-листе:
Соответствует ли код вашему стилю написания кода? Обычно это относится к расположению скобок, названиям переменных и функций, длинам строк, отступам, форматированию и комментариям.
Сами себе противоречат?
Я полагаю, что в качествен инструментов имелись ввиду две вещи: стандартные соглашения форматирования и автоформатирование в IDE.
Но на самом деле эти инструменты не исключают необходимость проверки. Очень часто бывает, что человек перед коммитом забыл вызвать функцию автоформатирования в IDE. Если это не контролировать и позволять изменять репозиторий, то следующий человек отформатирует код и в истории будет именно он, что ненамного, но затрудняет поиск реального автора кода.
Но на самом деле эти инструменты не исключают необходимость проверки. Очень часто бывает, что человек перед коммитом забыл вызвать функцию автоформатирования в IDE. Если это не контролировать и позволять изменять репозиторий, то следующий человек отформатирует код и в истории будет именно он, что ненамного, но затрудняет поиск реального автора кода.
Ну вот, чеклист уже можно оптимизировать:) Если на проекте используется автоматизированная проверка форматирования, то как минимум проверку скобок и отступов можно убрать. Но если включить в стиль названия переменных и функций, то автоматизировать это вряд ли удастся: вряд ли инструмент сможет распознать непонятное название переменной.
Идея хорошая, и я о ней узнал из книги 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
— все ли деструкторы методов с виртуальными функциями помечены как virtual? (да, это нужно не всегда и порой я вижу ситуации когда в этом нет необходимости)
— хорошо ли я понимаю кто владеет сырыми указателями и понимаю когда он объект будет удален
— хорошо ли я понимаю какая функция на каком потоке будет вызвана и что из этого следует
— есть ли проверки на разыменование указателей, могут ли указатели быть нулевыми?
— нет ли проблем с кодировками при переходе от string к wstring
del
А куда делась статья про эффективность код ревью? И почему?
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Уменьшаем количество ошибок с помощью чек-листа Code Review