Когда-нибудь мы все писали (а некоторые и пишут) плохой код, и, надеюсь, мы все работаем над улучшением наших навыков, а не просто чтением статей вроде этой.
Зачем нам писать хороший код, а не просто производительный код?
Хотя производительность вашего продукта или сайта важна, также важно и то, как выглядит ваш код. Причиной этого является то, что не только машина читает ваш код.
Во-первых, рано или поздно вам придется перечитывать собственный код, и когда это время наступит, только хорошо написанный код поможет вам понять, что вы написали, или выяснить, как это исправить.
Во-вторых, если вы работаете в команде или сотрудничаете с другими разработчиками, то все члены команды будут читать ваш код и пытаться интерпретировать его так, как они понимают. Чтобы сделать это проще для них, важно соблюдать некие правила при названии переменных и функций, ограничивать длину каждой строки и сохранять структуру вашего кода.
Наконец, давайте рассмотрим конкретный пример.
Часть 1: Как определить плохой код?
Самый простой способ определить плохой код, на мой взгляд, — попытаться прочитать код так, как если бы это было предложение или фраза.
Например, взглянем на этот код:
Скриншот плохой версии функции «traverseUpUntil»
Представленная выше функция принимает некий элемент и условную функцию и возвращает ближайший родительский узел, который удовлетворяет условной функции.
const traverseUpUntil = (el, f) => {
Исходя из того, что код должен читаться, как обычный текст, первая строка имеет три грубейших недостатка.
- Параметры функции не читаются, как слова.
- Допустим,
el
можно понять, поскольку такое имя обычно используется для обозначения элемента, но имя параметраf
не объясняет ровным счётом ничего. - Название функции можно прочитать так так: «переходить до тех пор, пока el не пройдет f», которое, вероятно, лучше читать как «переходить до тех пор, пока f не пройдет для el». Конечно, лучший способ сделать это — позволить функции вызываться как
el.traverseUpUntil(f)
, но это другая проблема.
let p = el.parentNode
Это вторая строка. Снова проблема с именами, на этот раз с переменной. Если бы кто-то посмотрел на код, то, скорее всего, понял бы, что такое p
. Это parentNode
параметра el
. Однако, что происходит, когда мы смотрим на p
, используемое где-либо еще, у нас больше нет контекста, который объясняет, что это такое.
while (p.parentNode && !f(p)) {
В следующей строке основная проблема, с которой мы сталкиваемся — это непонимание того, что означает или делает !f(p)
, потому что «f» может означать всё, что угодно. Предполагается, что человек, читающий код, должен понимать, что !f(p)
— это проверка текущего узла на удовлетворение определённому условию. Если она проходит, то цикл прерывается.
p = p.parentNode
Здесь всё понятно.
return p
Не совсем очевидно, что возвращается из-за неправильного имени переменной.
Часть 2: Давайте отрефакторим
Скриншот хорошей версии функции «traverseUpUntil»
Сначала мы изменяем имена параметров и их порядок: (el, f) =>
в (condition, node) =>
.
Возможно, вам интересно, почему вместо «element (рус. элемент) я использовал «node» (рус. узел). Я использовал его по следующим причинам:
- Мы пишем код в терминах узлов, например
.parentNode
, поэтому почему бы не сделать его консистентным. - «Node» короче, чем «element», и при этом смысл не теряется.
Затем мы переходим к именам переменных:
let parent = node
Очень важно полностью раскрыть значение вашей переменной в её имени, поэтому «p» теперь «parent» (рус. родитель). Возможно, вы также заметили, что теперь мы не начинаем с получения родительского узла node.parentNode
, вместо этого получаем только узел.
Идём далее:
do {
parent = parent.parentNode
} while (parent.parentNode && !condition(parent))
Вместо обычного цикла while
я выбрал цикл do ... while
. Это означает, что нам нужно каждый раз перед проверкой условия получать родительский узел, а не наоборот. Использование цикла do ... while
также способствует тому, чтобы читать код, как обычный текст.
Давайте попробуем прочитать: «Присвоить родительский узел родителя родителю, пока у родителя есть родительский узел, а функция условия не возвращает true». Уже гораздо понятнее.
return parent
Часто разработчики предпочитают использовать какую-то общую переменную ret
(или returnValue
), но это довольно плохая практика. Если вы правильно назовёте свои возвращаемые переменные, становится очевидным то, что возвращается. Однако иногда функции могут быть длинными и сложными, что приводит к большой путанице. В этом случае я бы предложил разделить вашу функцию на несколько функций, и если она всё ещё слишком сложна, то, возможно, добавление комментариев может помочь.
Часть 3: Упрощение кода
Теперь, когда вы сделали код читабельным, пришло время убрать ненужный код. Я уверен, что некоторые из вас уже заметили, что нам вообще не нужна переменная parent
.
const traverseUpUntil = (condition, node) => {
do {
node = node.parentNode
} while (node.parentNode && !condition(node))
return node
}
Я просто убрал первую строку и заменил «parent» на «node». Таким образом, я пропустил ненужный шаг создания «parent» и перешёл прямо в цикл.
Но что насчёт имени переменной?
Хотя «node» не лучшее описание для этой переменной, оно удовлетворительное. Но давайте не будем останавливаться на этом, давайте переименуем её. Как насчёт «currentNode»?
const traverseUpUntil = (condition, currentNode) => {
do {
currentNode = currentNode.parentNode
} while (currentNode.parentNode && !condition(currentNode))
return currentNode
}
Так-то лучше! Теперь, когда мы читаем метод, мы знаем, что currentNode
всегда представляет собой узел, в котором мы сейчас находимся, вместо того, чтобы быть «каким-то» узлом.
Комментарии (14)
max_dark
31.08.2018 14:23Вместо обычного цикла while я выбрал цикл do… while.
Замена ошибочна, так как условие цикла в исходном варианте может оказаться ложным уже на первом проходе, если
currentNode
является корнем либо не удовлетворяет условиюcondition
Тогда как второй вариант исполнит тело цикла как минимум один разdopusteam
31.08.2018 14:53+1Не зря говорят 'работает — не трогай'
Попытался сделать читабельнее метод из ~10 строк и даже при таком маленьком рефакторинге сломал что то
max_dark
31.08.2018 15:13Извиняюсь, невнимательно читал код.
В этом конкретном случае замена корректна, так как в первом варианте тело цикла совпадает по смыслу с кодом до цикла.
То есть было:
code; // какой то код while(...) { code; // тот же код }
Стало:
do { code; } while(...);
Тем самым убрали дублирование
shai_hulud
31.08.2018 14:52Если честно я закончил читать код сразу после «const traverseUpUntil = », зачем мне знать как оно сделано внутри, если по названию уже понятно, el и f для меня очевидны.
П.с. параметры на null никто и не проверил :)
peresada
31.08.2018 14:57Не говоря об ошибках, всю статью можно заменить на ссылку про самодокументируемый код
dipsy
31.08.2018 16:04А чем condition отличается по сути от f?
f может быть принятым в данном контексте (модуле, проекте), обозначением унарного предиката, просто и понятно.
Точно так же с p, в функции из трех строк заменять его на currentNode, серьёзно? Другое дело если функция на 1-2 экрана, и там реально можно забыть что за p появляется в конце функции, ну или в функции больше 2-3 переменных с однобуквенными именами.
Интересно, какой вариант вам больше нравится:
int sum(int x, int y) { return x + y } int calculateSum(int leftOperand, int rightOperand) { return leftOperand + rightOperand }
Devcolibri Автор
31.08.2018 16:11Это ведь просто пример на небольшом куске кода, который можно и нужно расширять на большие классы
dipsy
31.08.2018 16:33Ну да, основная идея должна быть «действуйте по обстоятельствам». И надо уменьшать энтропию в коде, если у вас узел ноды обозначается el, то он во всем модуле или проекте должен так обозначаться, чтобы не было где-то el, где-то node, а где-то currentNode. И, естественно, копипасты во всех видах надо убирать, от синтаксических до логических. И контекст учитывать, parentNode переименовать в parent, например, а то масло масляное и многабукф
currentNode = currentNode.parentNode vs node = node.parent
LeshaRB
01.09.2018 10:18Я не понимаю смысла данных статей…
Вы прочитали книгу "Чистый код, Рефакторинг" и делитесь выжимками?
Честно когда вы вели видеоблог и обычный блог, было лучше...
Devcolibri Автор
02.09.2018 17:29У нас и сейчас выходят видео и статьи в блоге, которые могут быть полезны сообществу
Если у вас есть какие-то конкретные пожелания/претензии/советы, то рады выслушать. Можно здесь, можно в личку
eskander_service
Писать на Java )
maxzh83
Да, сразу же про Java подумалось. И ее многословность часто подбешивает, как пример, реальное имя класса из недр Spring: HasThisTypePatternTriedToSneakInSomeGenericOrParameterizedTypePatternMatchingStuffAnywhereVisitor
Это конечно больше прикол, но вот такие имена SimpleBeanFactoryAwareAspectInstanceFactory встречаются часто. В случае с переменной получится совсем эпично (до появления var): имя длинного класса, потом имя переменной, которое генерится IDE по классу, и которое мало кто меняет, и наконец конструктор — тоже имя длинного класса. Смотрится это все сразу серьезно, энтерпрайзно. Но так уж сложилось, ничего не поделаешь.
eskander_service
осталось ещё Objective-C передрать, и «программистами» называть только тех кто на этом пишет)