Плохой код #2
Комментарии в коде крайне важны. Еще важнее хорошие комментарии. Однако всегда ли они нужны? Стоит ли комментировать каждую строчку? Скорее всего нет. Но где проходит граница, что нужно комментировать, а что нет? И насколько подробным должен быть комментарий? В этой статье попробуем разобраться.
Попробуйте определить, нужны ли комментарии в каждом из отрывков кода, и если да, то насколько подробны они должны быть? Стоит ли что-нибудь убрать, изменить, добавить?
Для упрощения допутим, что функции в данном не связаны, т.е. не находятся в одном модуле, коде, и вообще не знают друг о друге. Не заморачивайтесь, стоит ли вместо Math.random использовать в них getRandomNumber - думайте о комментариях.
// Возращает случайное число
function getRandomNumber(){
return Math.random();
}
// Возращает случайное число от min до max
function getRandomInRange(min, max) {
return Math.random() * (max - min) + min;
}
function getRandomIntInRange(min, max){
return Math.floor(Math.random() * (max - min) + min);
}
// Возращает случайное число от min (включительно) до max
function getRandomIntInRangeInclusive(min, max){
return Math.floor(Math.random() * (max - min + 1) + min);
}В нижнем примере приведен только заголовок функции, ее реализация нас пока не интересует (но мы несомненно доберемся до нее).
/**
* Генерирует простые числа в диапазоне до максимального значения, заданного
* пользователем, по алгоритму "Решето Эратосфена".
* ---
* Эратосфен Киренский. 276 год до н.э., Ливия -- * 194 год до н.э., Александрия.
* Первый ученый, вычисливший длину земного меридиана. Известен своими работами о календарях
* о календарях с високосным годом, заведовал Александрийской библиотекой.
* ---
* Алгоритм весьма прост. Берем массив целых чисел, начиная с 2, и вычеркиваем из него все числа,
* кратные 2. Находим следующее невычеркнутое число и вычеркиваем все его кратные. Повторяем до тех
* пор, пока не дойдем до квадратного корня верхней границы диапазона.
*/
function generatePrimes(maxBound){
// ...
}И еще один, довольно абстрактный, но полезный пример.
/*
* Начать с массива, размер которого достаточен для хранения всех пикселов
* (плюс байты фильтра), плюс еще 200 для данных заголовка
* Инициализируем его максимумом.
*/
let pngBytes = [];
for(let i = 0; i < (width+1)*height*3) + 200; i++){
pngBytes.push(16777215);
}И напоследок
// Опыт для поднятия текущего уровня
function getExpForLevel(n){
// Сумма чисел от 1 до n умножить на "дополнительный коэффициент"
// и деленное на два
return n*(n+1) * Math.floor((n+10) / 10) / 2;
}Решение
Начнем попорядку - возьмем первый пример первую функцию.
// Возращает случайное число
function getRandomNumber(){
return Math.random();
}Полезен ли этот комментарий? Дает ли он новую информацию в дополнение коду? Нет. Он лишь создает шум. Он не объясняет, зачем нужна эта функция, когда есть Math.random, или не говорит, в каком диапазоне будет возвращаемое число. Первое может объясняться многим - возможно, Math.random это временный вариант (вот поэтому), или это лишь временная заглушка и скоро будет удалена. В любом случае, вариантов много, однако из комментария ничего не ясно. Диапазон возвращаемых чисел можно посмотреть в документации к Math.random, однако для нас это лишняя работа. И есть ли гарантии, что после переработки функции диапазон останется таким-же?
// Возращает случайное число в диапазоне [0, 1)
function getRandomNumber(){
return Math.random();
}Такой комментарий является напоминанием о возвращаем диапазоне. Позднее при переработке он попадется на глаза, и мы не забудем сохранить диапазон таким же.
Со второй функцией теперь все должно быть понятно.
// Возращает случайное число от min до max
function getRandomInRange(min, max) {
return Math.random() * (max - min) + min;
}Опять же не ясно, какой конкретно диапазон возвращаемых значений. Конечно, в программировании довольно часто фраза “от Х до Y” означает включая X и исключая Y, но это не очевидно сразу. Для определения диапазона приходится смотреть в код, а это явный признак плохого комментария.
// Возращает случайное число в диапазоне [min, max)
function getRandomInRange(min, max) {
return Math.random() * (max - min) + min;
}У третьей функции комментарий отсутсвует вовсе. С одной стороны, если вы прочитали весь код до этого, вам из названия должно быть понятно, что она делает. С другой стороны, читатель вашего кода может пропустить весь предыдущий код, воспользоваться поиском по тексту и прыгнуть сразу к этой функции. И опять, ему будет совсем не ясен диапазон значений.
function getRandomIntInRange(min, max){
return Math.floor(Math.random() * (max - min) + min);
}С одной стороны кажется, что плодить однотипные комментарии скучно и бессмысленно, с другой стороны getRandomInRange не зависит ни от какой другой функции, кроме Math.random, поэтому и комментировать ее стоит соответсвенно - независимо от других функций.
// Возвращает случайное целое число в диапазоне [min, max)
function getRandomIntInRange(min, max){
return Math.floor(Math.random() * (max - min) + min);
}Еще одно важное замечание - из-за использования Math.floor распределение чисел не будет непрерывным равномерным. Это может быть понятно человеку, только хорошо знакомому с теорией вероятностей, и мы не можем расчитывать, что каждый читатель нашего кода будет настолько хорош, что сможет определить, каким является распределение возвращаемых чисел с первого взгляда на код. Поэтому хорошей идеей будет добавить об этом комментарий.
/**
* Возвращает случайное целое число в диапазоне [min, max)
*
* Из-за использования Math.floor генерируемые числа
* не соотвествуют непрерывному равномерному распределению.
*/
function getRandomIntInRange(min, max){
return Math.floor(Math.random() * (max - min) + min);
}В четвертой функции сразу бросается слово включительно. Что оно означает? Что значение min включено в диапазон возвращаемых значений? Так было и для прошлых функций, почему это упомянуто только здесь?
// Возращает случайное число от min (включительно) до max
function getRandomIntInRangeInclusive(min, max){
return Math.floor(Math.random() * (max - min + 1) + min);
}Если посмотреть на код, видно, что возвращаются числа в диапазоне [min, max], то есть включая max. Скорее всего, автор хотел написать так.
// Возращает случайное число от min до max включительно
function getRandomIntInRangeInclusive(min, max){
return Math.floor(Math.random() * (max - min + 1) + min);
}Однако он поторопился, и вставил слово “включительно” не в то место, вызывая недоразумение у читателя.
Переходим ко второму примеру.
/**
* Генерирует простые числа в диапазоне до максимального значения, заданного
* пользователем, по алгоритму "Решето Эратосфена".
* ---
* Эратосфен Киренский. 276 год до н.э., Ливия -- * 194 год до н.э., Александрия.
* Первый ученый, вычисливший длину земного меридиана. Известен своими работами о календарях
* о календарях с високосным годом, заведовал Александрийской библиотекой.
* ---
* Алгоритм весьма прост. Берем массив целых чисел, начиная с 2, и вычеркиваем из него все числа,
* кратные 2. Находим следующее невычеркнутое число и вычеркиваем все его кратные. Повторяем до тех
* пор, пока не дойдем до квадратного корня верхней границы диапазона.
*/
function generatePrimes(maxBound){
// ...
}Сразу же хочется спросить - зачем нам знать подробности жизни Эратосфена? Безусловно это интересно и позновательно, однако никак не помогает понять код и только отвлекает от дела. Убираем сразу.
/**
* Генерирует простые числа в диапазоне до максимального значения, заданного
* пользователем, по алгоритму "Решето Эратосфена".
* Алгоритм весьма прост. Берем массив целых чисел, начиная с 2, и вычеркиваем из него все числа,
* кратные 2. Находим следующее невычеркнутое число и вычеркиваем все его кратные. Повторяем до тех
* пор, пока не дойдем до квадратного корня верхней границы диапазона.
*/
function generatePrimes(maxBound){
// ...
}После предыдущих примеров вы должны задать следующий вопрос - входит ли само значение maxBound в возращаемые значения? Это не ясно из названия функции, не ясно из комментария - придется лезть в код и искать эту информацию.
Более того, фраза “числа в диапазоне” может привести к путанице. Мы ведь задаем не весь диапазон, а только максимальное значение. Давайте перепишем эту фразу в ранее уже используемом стиле.
/**
* Генерирует простые числа в диапазоне [2, maxBound] по алгоритму "Решето Эратосфена".
*
* Алгоритм весьма прост. Берем массив целых чисел, начиная с 2, и вычеркиваем из него все числа,
* кратные 2. Находим следующее невычеркнутое число и вычеркиваем все его кратные. Повторяем до тех
* пор, пока не дойдем до квадратного корня верхней границы диапазона.
*/
function generatePrimes(maxBound){
// ...
}Теперь про описание работы алгоритма. Если код написан хорошо, такой комментарий излишен. Внутренние детали кода должны быть понятны из самого кода. Однако особое внимание стоит уделить строчке “Повторяем до тех пор, пока не дойдем до квадратного корня верхней границы диапазона”. Почему именно квадратного корня? Почему не до самого maxBound, или его логарифма, или половине? Комментарий ничего не объясняет, он констатирует факт. Плохой комментарий. Попробуем переработать.
// Генерирует простые числа в диапазоне [2, maxBound] по алгоритму "Решето Эратосфена".
function generatePrimes(maxBound){
// Если x кратно a: a < sqrt(x), то x кратно x/a > sqrt(x).
// Из этого следует, что все числа, кратные больше sqrt(x) мы вычеркнем,
// вычеркивая числа, меньшие sqrt(x). Следовательно, дальше sqrt(x) вычеркивать не надо.
let iterationLimit = Math.round(Math.sqrt(maxBound));
}Вынесем подробности про квадратный корень внутрь функции, а так же объясним причину, почему мы можем использовать именно квадратный корень как верхнюю границу итерации.
Третий пример.
/*
* Начать с массива, размер которого достаточен для хранения всех пикселов
* (плюс байты фильтра), плюс еще 200 для данных заголовка
* Инициализируем его максимумом.
*/
let pngBytes = [];
for(let i = 0; i < (width+1)*height*3) + 200; i++){
pngBytes.push(16777215);
}Комментарий вроде как все объясняет, особенно человеку, который разбирается в проекте и знает весь контекст. Однако для нас он бесполезен более чем полностью. Что такое “все пиксели”. Все пиксели чего? Экрана? Картинки? Текущего кадра? Скорее всего картинки, так как мы работаем с массивом pngBytes, однако сказать точно сложно. Что такое байты фильтра? Я не вижу, чтобы они где-то прибавлялись. width+1 это байты фильтра? Не похоже, ведь это только один байт. Может тогда умножение на 3 это байты фильтра? Возможно, но тогда почему именно на 3? Может ли это число изменится со временем? Скорее всего да, однако комментарий не объясняет ничего. И что такое width+1? Почему +1? Может пиксели нумеруются с 1, поэтому нужно это учесть? Или это это все-таки пресловутые байты фильтра? И что такое 200 байт для данных заголовка? Какого заголовка, почему именно 200? Это число вполне может измениться, так почему оно вставлено прямо в код? Это плохой код, или плохой комментарий? Скорее всего и то и то. И что за ужасное число “16777215”? Почему оно называется максимумом? Максимумом чего? Это явно не максимальное целое число, однако ничего другого на ум не приходит.
Мы явно имеем плохой код, и плохой комментарий не дает ответов, а только приводит к большим вопросам. Двойное комбо. Попробуем его разобрать.
Попробуем переработать и код и комментарий. Сначала код. Проблема у него одна - магические константы. Лучшая версия кода могла выглядеть так.
let pngBytes = [];
let leftMargin = 1;
let pixelSize = 3;
let headSize = 200;
let whiteColor = 0xffffff;
for(let i = 0; i < (width+leftMargin)*height*pixelSize) + headSize; i++){
pngBytes.push(whiteColor);
}Конечно, скорее всего размер пикселя в байтах, размер заголовка, сдвиг слева, белый цвет - все эти числа относятся не к данному участку кода и их стоит перенести в более подходящее место. Размер заголовка должен относится к классу заголовка, белый цвет определен в модуле цветов и т.д. Однако главную задачу мы выполнили - код стал понятнее. Теперь встает вопрос, а нужен ли комментарий вообще? Скорее всего нет, разве что стоит описать, зачем нам нужен pngBytes, если это не ясно из контекста. Но скорее всего ясно - мы ведь пишем хороший код? Таким образом, код становится комментарием самому себе.
И последний пример, в котором никто из комментаторов статьи не обнаружил главную ошибку.
// Опыт для поднятия текущего уровня
function getExpForLevel(n){
// Сумма чисел от 1 до n умножить на "дополнительный коэффициент"
// и деленное на два
return n*(n+1) * Math.floor((n+20) / 10) / 2;
}Вспомним формулу суммы чисел от 1 до n. После недолгого поиска в гугле, находим n*(n+1)/2. Однако, судя ко комментарию, сумма чисел от 1 до n равна n*(n+1), а деление на 2 - отдельная операция. Получается, либо комментарий, либо код врет. Знать наверняка мы не можем, нужно спрашивать автора кода. Предположим, комментарий врет.
// Опыт для поднятия текущего уровня
function getExpForLevel(n){
// Сумма чисел от 1 до n умножить на "дополнительный коэффициент"
return n*(n+1)/2 * Math.floor((n+20) / 10);
}Выглядит несомненно лучше. Однако, попробуем сделать проще.
// Опыт для поднятия текущего уровня
function getExpForLevel(n){
let sumFromOneToN = n*(n+1)/2;
return sumFromOneToN * Math.floor((n+20) / 10); // дополнительный коэффициент
}Теперь нам практически некуда вставить комментарий про дополнительный коэффициент. Попробуем также вынести его в переменную.
// Опыт для поднятия текущего уровня
function getExpForLevel(n){
let sumFromOneToN = n*(n+1)/2;
// Дополнительный коэффициент
// Повышает сложность прокачки на более высоких уровнях
let coef = Math.floor((n+20)/10);
return sumFromOneToN * coef;
}Теперь код становится еще понятнее, и у нас остается много место для описания дополнительного коэффициента. До этого было совсем не понятно, зачем он нужен.
Комментарий, описывающий саму функцию, можно оставить.
Итог
Комментарии являются крайне важной частью вашей программы. Они помогают читателю вашего кода понять его предназначение, быстрее “влиться” во все его аспекты, объяснить непонятные технические детали. Однако они же могут препятствовать понимаю кода. Они могут создавать текстовый шум, они могут сбивать читателя столку, они могут нагло врать. Крайне важно тщательно писать ваши комментарии, стараясь не просто описывать, что делает ваш код, а почему он это делает, и почему именно таким способом.
Часто во время обсуждения поднимается технический вопрос вроде “что использовать в качестве верхней границы, x или sqrt(x)?”. В итоге принимается решение и оно переносится в код. Именно такие решения важно комментировать, потому что не зная всех аспектов, которые были обсуждены, невозможно понять, почему же именно sqrt(x)? Хуже всего, если через полгода обсуждение забывается, его участники натыкаются на код, и опять начинается обсуждение “мы тут sqrt(x) используем, это ошибка, или намерение? Вроде x использовать лучше”. При наличии комментария таких проблем легко избежать.
В следующей статье мы рассмотрим аспект, так же сильно влияющий на понимание кода - имена переменных, функций и классов.
Спасибо!
Комментарии