egorF 4 сен 2012 в 19:03

Главный принцип хорошего кода

9 мин

86K

Программирование * Проектирование и рефакторинг * Веб-разработка *

+72

174

Комментарии 174

DarthRamone 4 сен 2012 в 19:13

Одно и тоже, одно и тоже. Сколько можно обсасывать одну и ту же конфетку.

1) Разделяй и властвуй.
2) Самокомментируемый код.
3) Понимание того, что пишешь.

Seter17 4 сен 2012 в 20:36

Повторение — мать заикания. Чем больше статей такого рода, тем чаще они будут попадаться на глаза новичкам и тем кому они нужны. Все новое — хорошо забытое старое. А правила хорошего кода — постоянно забывают.

Colwin 5 сен 2012 в 06:52

Не забывают, а игнорируют, что печальнее.

LionMuzzle 5 сен 2012 в 10:00

Подтверждаю! Сколько раз своим на работе говорил, что такая техника только на пользу идет. Отмахиваются, мол мне и так хорошо. И, мол, всех слушать — только себе во вред, типа у каждого свои советы, и мол буду делать так, как привык.

Короче, как я понял, есть творческие программисты, а есть программисты-исполнители. Первые всегда открыты для новых методик, ищут, жаждут, как улучшить качество своего кода. Вторым просто нужно выполнить работу, не важно, каким способом.

VolCh 5 сен 2012 в 10:25

Исполнительные как раз по идее должны делать тем способом, которым укажут. Самые «опасные», имхо, творческие, но без критического взгляда на свой код — такого натворят :)

LionMuzzle 5 сен 2012 в 11:04

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

VolCh 5 сен 2012 в 11:38

Ну, если надо постоянно рядом сидеть, то значит не такой уж исполнительный :)

NorthernEyes 5 сен 2012 в 16:33

1. Рефаторинг — это именно то, что проделано в этой статье. Для этого существует замечательная книжка «Рефакторинг» Фаулера.
2. «Совершенный код» Макконнелла, в которой приведено множество замечательных методик проектирования (и многого другого), чтобы по возможности сразу писать хороший код.

lair 5 сен 2012 в 16:48

Не совсем так. Для рефакторинга в этом коде не хватает одной, но очень важной вещи — тестового покрытия.

NorthernEyes 5 сен 2012 в 16:49

Совершенно согласен.

VolCh 5 сен 2012 в 17:24

Есть варианты, когда рефакторинг можно осуществлять и без 100% покрытия тестами.

lair 6 сен 2012 в 06:56

А как вы гарантируете неизменность поведения (а это основное условие рефакторинга)?

VolCh 6 сен 2012 в 11:41

Тривиальностью рефакторинга или тем, что он делается не ручками, а средствами IDE.

lair 6 сен 2012 в 11:48

Ни то, ни другое не является гарантией.

(если что, проверено на практике)

VolCh 6 сен 2012 в 12:00

Мне на практике хватает. Иначе рефакторинг унаследованного кода был бы очень-очень затруднён.

lair 6 сен 2012 в 12:05

Вообще-то, в «вашей настольной книге» рекомендуют сначала разорвать зависимости, затем покрыть тестами, а затем рефакторить.

VolCh 7 сен 2012 в 10:48

Разрыв зависимостей разве не является рефакторингом?

lair 7 сен 2012 в 10:56

Да, был не совсем прав, извиняюсь.

Сначала покрыть тестами (интеграционными), потом разорвать зависимости, потом покрыть юнит-тестами, потом рефакторить все остальное.

VolCh 7 сен 2012 в 11:15

Вот первым этапом я зачастую пренебрегаю, ведь для того чтобы понять что писать в тестах нужно разобраться с кодом, а чтобы разобраться с кодом желательно его привести к читаемому виду. Написать интеграционные/функциональные тесты на god-скрипт в несколько тысяч строк, в один «поток» делающий всё от анализа параметров окружения до работы с БД, проблематично. Приходится, как минимум, выделять функции, а то и рвать зависимости без покрытия тестам, а уж потом писать тесты.

lair 7 сен 2012 в 11:20

Вот и зря пренебрегаете. Потому что тесты пишутся по наблюдаемому поведению (и зачастую не класса, а системы целиком), и разбираться в коде для этого не надо.

VolCh 7 сен 2012 в 11:52

Про классы речь и не идёт, именно о системе целиком. А без чтения кода очень затруднительно понять возможные варианты поведения. Система многопользовательская, с активным взаимодействием пользователей, кучей откладываемых событий (обрабатываются асинхронно, «он деманд»), которые обрабатываются перед собственно выполнением запроса и от результата (и порядка) обработки которых могут зависеть (а могут и нет) результаты проверки пары десятков взаимозависимых пред- и постусловий действия. В общем возможных исходов у обработки одного запроса могут быть десятки, как минимум. Покрывать для каждого возможного запроса все возможные результаты не выглядит разумным. Лучше сначала разорвать зависимости и тестировать более-менее изолировано.

lair 7 сен 2012 в 12:03

Звучит так, как будто вы вообще эту систему не тестируете. Что, прямо скажем, грустно.

VolCh 7 сен 2012 в 12:10

Тестируем то, в чём разобрались. А чтобы разобраться приходится проводить более-менее безопасный рефакторинг без тестов, покрывая ими только изменения логики, но не формы её представления.

lair 7 сен 2012 в 12:12

Ну то есть систему — не тестируете (поскольку в самой системе не разобрались). Это и грустно.

VolCh 7 сен 2012 в 12:23

А что делать. Надо сначала её к тестируемому виду привести.

lair 7 сен 2012 в 12:25

Покрывать верхнеуровневыми тестами, понятное дело. Это в любом случае надо делать, на самом деле, одними юнит-тестами жив не будешь.

VolCh 7 сен 2012 в 13:08

Замкнутый круг:

— чтобы привести приложение в поддерживаемое (читабельное прежде всего) состояние, нужно покрыть его тестами

— чтобы покрыть тестами нужно знать что идёт на вход (в моём случае: данные http-запроса, состояние php-сессии и БД) и что должно быть на выходе (http-ответ и изменение состояния БД).

— чтобы знать что должно быть на выходе, нужно изучать код (документации никакой)

— чтобы изучать код, нужно чтобы он был читабельный.

Этот круг надо где-то разрывать. Я решил, что наименее затратно выделять элементарные куски кода в функции, по возможности в чистые (нечистую часть максимально изолировать), которые а) легко покрываются юнит-тестами, б) обладают простой логикой и имеют единственную ответственность (как следствие — легко именуются) и в) из общего кода относительно безопасно выделяются без тестов.

Когда в довольно большом куске оригинального кода остаются практически только вызовы таких функций (покрытых юнит-тестами) и логика этого куска становится понятна, то пишу интеграционные/функциональные тесты, уже без необходимости тестировать работу «низкоуровневых» ветвлений и циклов, после чего можно как рефакторить, так и изменять логику, без страха не учесть побочный эффект в другом месте. Ну или при получении баг-репорта быстро локализовать проблему и понять где всё-таки умудрился понять логику неверно.

lair 9 сен 2012 в 16:00

Да нет никакого замкнутого круга. Если вы вообще не знаете, как себя должна вести система, просто не надо ее трогать.

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

VolCh 9 сен 2012 в 16:59

Нельзя сказать, что совсем не знаем. Общее представление конечно есть, нет понимания алгоритмов, формул, путей прохождения событий и т. п. А часть параметров — текущее время, как в основном коде, так и в SQL, а часть вообще рандомные, то есть даже контрольная копия не поможет для нормального тестирования. Нужно рвать зависимости от rand(), time() и NOW() чтобы было что в тестах сравнивать.

lair 9 сен 2012 в 17:10

Общее представление конечно есть, нет понимания алгоритмов, формул, путей прохождения событий и т. п.

А какая разница? Мы же говорим о внешнем наблюдаемом поведении.

Нужно рвать зависимости от rand(), time() и NOW() чтобы было что в тестах сравнивать.

Все, кроме rand(), лечится сравниванием с tolerance.

VolCh 9 сен 2012 в 17:30

Потому что внешнее поведение случайно и не только числа могут быть разные. Грубо говоря, представьте, что у квадратного уравнения случайный дискриминант: может быть два вещественных корня, может быть один, может быть два комплексных, для каждого свой экран вывода.

Про tolerance не понял, можно по-русски?

lair 9 сен 2012 в 17:40

Потому что внешнее поведение случайно

Что это за система такая, простите? Какую бизнес-задачу она решает?

Про tolerance не понял, можно по-русски?

Если у вас есть алгоритм, зависимый от текущего времени, то вы можете проверять, что результат теста отличается от контрольного значения не больше, чем время на момент теста отличается от времени замера контрольного значения. Плюс разбег на границы самого теста.

VolCh 13 сен 2012 в 12:26

ММО RTS

Алгоритм зависит от разницы текущего времени и времени какого-то ранее записанного в базе события. С трудом представляю как это можно тестировать с помощью снимков и логирования запросов и ответов.

lair 13 сен 2012 в 12:36

ММО RTS

Там поведение не случайно, а выглядит случайным.

С трудом представляю как это можно тестировать с помощью снимков и логирования запросов и ответов.

Конкретно это — нельзя. Здесь нужен дополнительный анализ.

VolCh 13 сен 2012 в 13:30

Ну да, выглядит случайным, на самом деле используется генератор псевдослучайных чисел :)

lair 7 сен 2012 в 12:26

lair 7 сен 2012 в 12:13

Ну то есть систему — не тестируете (поскольку в самой системе не разобрались). Это и грустно.

VolCh 7 сен 2012 в 11:53

VolCh 7 сен 2012 в 11:54

VolCh 7 сен 2012 в 12:02

Блин, хабр глючит :(

lair 7 сен 2012 в 11:30

Кстати, у этой проблемы есть тривиальное решение, называется «контрольная копия» (и сравнение поведения на большой выборке данных). Мы так рефакторим (до сих пор) некий god-класс.

VolCh 7 сен 2012 в 12:01

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

lair 7 сен 2012 в 12:04

Э, а кто вообще занимается рефакторингом меняющегося кода?

VolCh 7 сен 2012 в 11:16

VolCh 7 сен 2012 в 11:17

newpavlov 4 сен 2012 в 19:13

if ($device->token != "" && $device->expire <= $now) {
    return true;
} else { 
    return false;
}

FACEPALM.JAR

egorF 4 сен 2012 в 19:14

там ниже написано, почему так.

newpavlov 4 сен 2012 в 19:20

Что-то я не найду где там написано почему мы не можем написать просто:

return ($device->token != "" && $device->expire <= $now);

Если ваша методика написания «легкочитаемого» кода предлагает мне писать (и читать) четыре строчки вместо одной, предлагает писать функции на каждый элементарный чих, то, извиняюсь, в топку такую методику.

Mrrl 5 сен 2012 в 04:41

Возможная причина — так можно на return false поставить breakpoint, чтобы потом разобраться в отладчике, почему это девайс не обнаруживается. Проще, чем вспоминать условные брекпойнты.

atomAltera 5 сен 2012 в 05:57

Слушай, гений, там принцип демонстируется, вместо return может код быть какой-то, что, обезательно пояснять для особо одарённых

newpavlov 5 сен 2012 в 15:04

Во-первых, прошу проследовать во вконтактики и прочие не особо отдалённые места рунета, в которых допустима продемонстрированная вами лексика и орфографические ошибки.
Во-вторых, в статье, претендующий на проповедование культуры кода, такие откровенно индусские участки абсолютно недопустимы.
В-третьих, статья автора, на мой скромный взгляд, состоит из давно известных истин и приправлена сомнительного качества примерами.

aav 4 сен 2012 в 19:22

Кстати, именно поэтому я пишу if (…) return true else return false. Можно было бы просто return (…), но тогда код было бы чуть сложнее читать, а возвращаемый тип не был бы мгновенно очевиден с первого взгляда.

А по названию функции возвращаемый тип не очевиден? Если очевиден, то что стало легче читать?

egorF 4 сен 2012 в 19:23

Если очевиден то, конечно же, не нужно писать такую многоэтажную конструкцию. Это вопрос вкуса. Где-то такая конструкция мешает, где-то напротив, позволяет с одного общего взгляда на код увидеть возвращаемый тип.

GetWindowsDirectory 4 сен 2012 в 19:23

Не проще ли так?


return ($device->token != "") && ($device->expire <= $now);

LestorN 4 сен 2012 в 19:29

Странно там написано. Человек не читает слова ни по буквам ни по слогам. В каждом языке (не только ЯП) есть свои конструкции, которые являются его частью и которые обязательно нужно знать. Да и return a == b читается намного быстрее. Доводы о том что это сложно воспринять заставляют вспомнить байку про


#define BEGIN {

#define END }

frig 4 сен 2012 в 19:18

В итоге все сводится к тому, что

Здесь нужно выработать вкус и тонкое чувство наития, подсказывающее, где стоит выделять код, а где нет.

А как выработать вкус и чувство наития в статье не объясняется.

Andy_Ion 4 сен 2012 в 19:42

Я в последнее время сильно рвусь между обычными foreach с if внутри и однострочным LINQ :(

Mithgol 5 сен 2012 в 07:57

А как выработать вкус и чувство наития в статье не объясняется.

Вкус и чувство наития вырабатывается эмпирически-концептуально, то есть путём естественного обобщения собственного опыта.

foxmuldercp 4 сен 2012 в 19:26

Спасибо за пару полезный советов. вооружусь на будущее:)

Pilat 4 сен 2012 в 20:01

Мне кажется, тривиальность исходных примеров вызвала непринятие методики. Действительно, из одной строки получили страницу кода — ужас. Но если бы исходная процедура занимала строк 100 кода, наверняка вопросов было бы меньше.

egorF 4 сен 2012 в 20:03

Верно, примеры совершенно синтетические. Но сто строк кода в статью тоже ведь не положишь. :)

LuciferOverLondon 6 сен 2012 в 15:14

У меня в последнее время такое ощущение, что, пока не положишь стопятьсот строк кода, до них не дойдет.

Pilat 4 сен 2012 в 21:24

Тут ещё есть пропущенный фактор — удобство написания тестов. Для кода из мелких фрагментов, наверно, тесты писать сильно проще.

VolCh 4 сен 2012 в 21:29

Тесты писать проще для изолированных фрагментов, их размер особой роли не играет. Например, в текущем проекте у меня куча функций/методов на 5-10 строк, которые тестами покрыты лишь в составе более крупных. Собственно сначала куски по сотне строк покрывал тестами, а потом эти функции выделял, убеждаясь, что тесты не ломаются.

mihaild 4 сен 2012 в 20:16

Теперь девайс отвечает на вопрос, можно ли его найти

Вот это мне не нравится. Класс девайса не должен реализовывать все мыслимые алгоритмы для работы с девайсом. Стоило бы сделать отдельную функцию.

fleshy 5 сен 2012 в 14:48

если эта функция будет требовать для работы private данные класса (а токен и время его истечения к таковым я бы отнес), то это плохая функция
device->isLocatable() мне кажется более правильным, чем isLocatable(device), т.к. locatability зависит от внутренних свойств экземпляра device, значит логичнее спросить об этом сам экземпляр

mihaild 5 сен 2012 в 14:53

Зависит от конкретики — что тут значит токен, как он работает, и т.д.
Вполне возможно, что в данном случае эту функцию действительно стоит включить в класс. Но в типичном случае лучше подходит отдельная функция.

VolCh 5 сен 2012 в 17:28

Это просто первая, самая простая стадия рефакторинга, практически гарантирующая неизменение логики без покрытия тестами.

spiritedflow 4 сен 2012 в 20:24

> Этот код уже не требует анализа — его достаточно лишь только прочитать.

Тут что-то не так. Это только в примере три получившиеся функции идут одна после другой. В реальном проекте они будут раскиданы по файлам, а кодер увидит только это:

function isLocatable() {
    if ($this->isOurTokenValid() && !$this->isDeviceExpired()) {
        return true;  // explicitly boolean
    } else { 
        return false;
    }
}

Смотря только на это место, всё равно непонятно, что такое токен, как считается Expire, что означает isOurTokenValid (и зачем там кстати Our? бывают ещё и не our?). Без ресёча глубокого понимания всё равно не будет.

egorF 4 сен 2012 в 20:24

Все правильно. Видите — есть куда стремиться!

spiritedflow 4 сен 2012 в 20:26

И ещё здесь нарушена инкапсуляция логики в объектах.
Не $this->isOurTokenValid() а $token->isValid().
Не $this->isDeviceExpired() а $device->isExpired().

rumkin 5 сен 2012 в 09:50

У вас так же непонятно что именно истекло! Какое событие было пропущено? Что если появится еще одно событие зависящее от времени? Правильней было бы создать метод isSessionExpired().

eugenius_nsk 5 сен 2012 в 05:29

Смотря только на это место, всё равно непонятно, что такое токен, как считается Expire, что означает isOurTokenValid

А зачем вам это знать в данном месте? Здесь хочется знать только то, как определяется, какие девайсы можно найти. При этом правила определения валидности токена могут потом измениться, но (обратите внимание!) критерии находимости девайса не изменятся.

spacediver 5 сен 2012 в 05:58

по-моему, «раскиданы» — не проблема, особенно если речь идет о разных уровнях абстракции. Кликнул, углубился, прочитал детали.

Лично я по этому коду понимаю, что locatable-ность зависит от валидности токена и expire-нутости устройства. Я запомнил, что эти штуки есть, и они как-то считаются. Когда в другом месте встречу «token» вспомню, для чего он еще был нужен. Вобщем, необходимый и достаточный объём сведений для прочтения без переходов по ссылкам.

exvel 5 сен 2012 в 08:02

Это только в примере три получившиеся функции идут одна после другой. В реальном проекте они будут раскиданы по файлам

Поэтому хорошая IDE так же важна.
* написал нежно поглаживая F12

VolCh 4 сен 2012 в 20:46

Кстати, именно поэтому я пишу if (…) return true else return false. Можно было бы просто return (…), но тогда код было бы чуть сложнее читать, а возвращаемый тип не был бы мгновенно очевиден с первого взгляда.

return (boolean)(...); или return boolval(...);, имхо, читается всё же проще, даже если boolval($val) нужно определить ручками как { if ($val) return true; else return false; }. На одно минимум переключение контекста меньше, по-моему, и просто DRY и следование вашим же принципам — одним словом описали что выражение делает :).

egorF 4 сен 2012 в 20:46

О сколько нам открытий чудных готовит PHP. ©

TIL про boolval().

m_z 4 сен 2012 в 21:00

По-моему, это бред. Тогда для консистенции придется везде выставлять return (string), return (array) и т.п.

Чтобы показать, что метод возвращает boolean есть два более простых способа: префикс «is» и/или "* return boolean".

egorF 4 сен 2012 в 21:01

В своем коде я пользуюсь префиксами «is» и подобными.

nazarpc 4 сен 2012 в 21:27

Используйте PhpDoc секции, наконец, с описаниями входных параметров и возвращаемых значений, ну, или аналоги в других языках, а не извращайтесь велосипедами. А авто дополнение IDE с такой документацией спасет вам кучу времени.

VolCh 4 сен 2012 в 21:35

Есть функции, которые возвращают, например, число или boolean. Анализ их результата выглядит примерно так:

if ($result === true) {
  echo "Positive infinity";
} elseif ($result === false) {
  echo "Negative infinity";
} else {
  echo $result;
}

Я уж молчу про функции, которые возвращают false или null (а null это отдельный тип) в случае ошибки, а если ок, то число, строку или объект.

mihaild 4 сен 2012 в 23:11

Функции, которые возвращают иногда int, а иногда bool — ошибка дизайна. Семантически у функции должен быть четко определенный тип возвращаемого значения.

VolCh 4 сен 2012 в 23:16

Увы, но в PHP это сплошь и рядом, начиная со стандартной библиотеки.

mihaild 4 сен 2012 в 23:18

Ага, strpos, возвращающий то 0, то false — это гениально.
Половина встроенных функций PHP выглядит ужасно, это так. Но зачем так писать самому, за исключением случаев, когда по какой-то причине нужно поддерживать [несуществующий] стиль стандартной библиотеки?

VolCh 4 сен 2012 в 23:23

Не знаю как вы, но я воспринял пост как советы о том, как работать с, прежде всего, чьим-то непонятным кодом. Как его приводить к читаемому виду без 100% покрытия тестами на все случаи жизни. Как не трогая логику подготовиться к её более-менее безопасному изменению.

LionMuzzle 5 сен 2012 в 10:17

Согласен. А об ошибках можно сообщить с помощью исключений.

m_z 4 сен 2012 в 21:01

не туда %(

VolCh 4 сен 2012 в 20:55

А вот серьёзная проблема при работе с унаследованным кодом это, когда выделяешь кусок строк в 5-10 в отдельную функцию и не понимаешь, что этот кусок кода делает. Синтаксис понимаешь, а вот семантику нет. Особенно если это работа с БД вида

SELECT * FROM table JOIN ... WHERE ... AND NOT ...

через переменные с именами вроде a, b, c, и названиями полей таблицы (которое смотришь в sql-дампе или через

SHOW TABLE ...

) на неидентифицируемом языке (в смысле человеческом, а не ЯП, но явно не английский и не русский транслит, только по мылам коммитеров можно догадаться что в проекте кроме английского, может быть немецкий, французкий и русский).

egorF 4 сен 2012 в 20:58

Такое не лечится.

VolCh 4 сен 2012 в 21:27

Ещё забыл упомянуть, что результаты запроса хранятся в глобальном ассоциативном массиве с именем типа rows, которому по проекту с сотню присваиваний и с тысячу использований, в основном вида

foreach ($rows as $row) echo "<li>$row['anz']</li>";

Лечится, но с большим трудом, приходится ручками в базе править данные, чтобы получить возможность выполнить этот кусок и понять что же он делает. Хорошо если с первого раза понятно какие какие данные в БД нужно ввести, чтоб достучаться до него.

LuciferOverLondon 6 сен 2012 в 15:23

Переходный этап: class SomeModel { const FIELD_ASIMPLECOLUMNMYGODWHYMUSTITBESOHARD = 'egyszerű'; }

tangro 4 сен 2012 в 21:07

Подход с написанием

if ($this->isOurTokenValid() && !$this->isDeviceExpired()) {
return true; // explicitly boolean
} else {
return false;
}

хуже, чем return в одну строку сразу по десятку причин:
1. Больше строк.
2. Больше сущностей.
3. Дольше время работы (компилятор может соптимизировать, но тут уж как ему хочется)
4. Последние 4 строки нужно либо копипастить (а значит легко ошибиться), либо писать руками, что долго
5. Комментарий напротив «true» о том что это тип boolean — это просто отлично. Для слепоглухонемых и тупых кодеров, видимо?
6. Хуже понимается — проговорите в уме (вслух, напишите на листочке) слова, которые вы думаете в обоих вариантах, когда читаете код. В многострочном варианте слов больше.
7. Если так написаны ВСЕ функции — элементарный код будет занимать тысячи строк
8. В мире С\С++ программирования такой подход вообще засмеют. Там любят лаконичность.
9. Код имеет две точки выхода, что, при прочих равных, хуже чем одна
10. Самая главная сложность кода — часть конструкции "() && !$" — шесть спецсимволов подряд. Именно эта часть содержит 95% сложности понимания этого выражения. Вместо разбиения кода на две строки или замены оператора "!" на "== false" автор кода экономит на спичках и занимается какой-то фигней.

Думаю, смог бы написать еще десяток причин при желании.

egorF 4 сен 2012 в 21:13

Ну все, проапдейтил статью и примеры.

tangro 4 сен 2012 в 21:15

и ведь лучше стало

egorF 4 сен 2012 в 21:15

Yep.

alisey 4 сен 2012 в 21:44

Сам пишу в стиле `return $foo && !$bar`, но вот честно, для уставшего слегка поддатого мозга начальный вариант читался понятнее.

Agel_Nash 5 сен 2012 в 04:43

Дело привычки. У себя в коде нашел подобие "() && !$" и как-то не задумывался раньше, что эту запись можно привести к более читабельному виду. Видимо ошибался.

LuciferOverLondon 6 сен 2012 в 16:05

> 9. Код имеет две точки выхода, что, при прочих равных, хуже чем одна

Ложь.

m_z 4 сен 2012 в 21:29

Да, и картинка должна быть вот эта:

В этой книге большинство приёмов рефакторинга описано, в т.ч и этот.

VolCh 4 сен 2012 в 21:39

Моя настольная книга:

spacediver 5 сен 2012 в 06:03

О, спасибо за ссылку.

TheHorse 4 сен 2012 в 21:35

function grabConfiguration() {
    if ($this->hasOption('configs')) {
        return $this->getOption('configs');
    } else if ($this->hasOption('config')) {
        return [ $this->getOption('config') ];
    }
}

Макконнелл среди прочего, говорил о том, что когда пишешь функцию, нужно мысленно пробежаться по всем вариантам ее работы, зайти в каждый if, в каждый else… Короче, вот что будет если и configs и config отсутствуют? Что будет с таким кодом:

$configs = grabConfiguration();
$configs->get("lol");

Глобально, это мелочь, которая правится практически мгновенно, но в статьях о качестве кода, так делать нельзя.

P. S. Надеюсь моих скудных знаний php хватило, чтобы не ошибочно указать на ошибку.

TheHorse 4 сен 2012 в 21:50

Прошу прощения, ошибочно указал на ошибку. Невнимательность виной тому, а не скудность знаний.

TheHorse 4 сен 2012 в 22:01

А еще невнимательность является виной предыдущему комментарию. Я умудрился запутаться в вашем простом коде, там явно не хватает

else {
   return null;
}

И вопрос на тему остается открытым, только не grabConfiguration, а getConfig.

P. S. Код с конфигурациями, вне контекста того где он был, плохой и до правок и после. Совсем не понятны сути тех сущностей.

VolCh 4 сен 2012 в 23:18

php > function a() {};
php > var_dump(a());
NULL
php >

TheHorse 4 сен 2012 в 23:38

Я подозревал что-то такое, но автор говорил о явности, удобочитаемости…

VolCh 5 сен 2012 в 00:28

С этой точки зрения да, формально нужно else return null; написать. С другой стороны даже python, где принцип «явное лучше неявного» возведён чуть ли не в абсолют, выводит

>>> def a():
...   pass
... 
>>> print(a())
None
>>>

Имхо, return null нужно писать, если функция подразумевает возврат значения (не процедура не терминах Паскаль), но этот return не последний значимый оператор. Если последний, то можно опустить и null, и сам return.

А если уж всё явно описывать то функция должна быть такой:

function grabConfiguration() {
    if ($this->hasOption('configs')) {
        $configs = $this->getOption('configs');
    } else if ($this->hasOption('config')) {
        $configs = [ $this->getOption('config') ];
    } else {
        $configs = null;
    }
    return $configs;
}

lalaki 5 сен 2012 в 08:41

А если прикинуть, что это реальная задача, то в случае отсутствия конфигов, скорее, exception надо кидать.

egorF 5 сен 2012 в 08:41

Это синтетический пример.

lalaki 5 сен 2012 в 08:45

Это понятно, поэтому я это не к самому топику написал, а к рассуждениям коллег: если уж доделывать до конца, то не return null, a throw. Добавил, так сказать, свою лепту занудства =)

TheHorse 5 сен 2012 в 09:45

Ну я бы не throw делал, а вернул бы в initialize false).

VolCh 5 сен 2012 в 10:23

Я бы бросал исключение уровнем выше, если бы вообще бросал. А вместо null возвращал бы пустой массив, а там пускай приложение думает, что делать если конфигов нет, исключительная ситуация или можно, например захардкоженные дефолтные данные загрузить.

Renius 5 сен 2012 в 04:29

Макконнелл возможно, в чем-то, и прав. Я согласен делать это только подсознательно.
Тесты с этим справляются эффективнее. Да и TDD пока никто не отменил.

alisey 4 сен 2012 в 21:38

Обычно методы с 'and' в названии — это code smell. Я бы сделал $device->notify(). Функция сама «достает» из device все данные, необходимые для уведомления. Для этого на может вызывать locate() или не вызывать, это деталь реализации, которой незачем торчать наружу.

Насчет главной идеи поста — выработал такую эвристику: я обычно разрабатываю снизу вверх, и когда у меня есть минимальный кусок кода, который можно тестировать независимо от всего остального — то выношу в отдельную функцию, даже если я не планировал ее создавать, и она не нужна в финальном API. Таким образом получается писать по 300-400 строк кода без багов. Запускаешь программу — и она просто работает. Вторая эвристика — не выносить дублирующийся код сразу, а подождать, пока наберется несколько похожих вариаций. Часто оказывается, что есть лучший способ убрать дублирование, чем я предполагал изначально, и предждевременная попытка это сделать не дала бы мне увидеть такую возможность. И третья эвристика — не разбивать методы исключительно ради разбивки. Часто один длинный метод читается лучше и понятнее, чем тот же метод разбитый на несколько маленьких. В оличие от автора поста я выношу что-то, только если оно начинает мозолить глаза и мешать.

Искал сейчас источник, но не нашел, исследование показывало, что оптимальный размер метода — от 30 до 50 строк. Размер больше 120 строк коррелирует с большим количеством багов, а множество мелких методов по 5 строк делает программу более сложной для понимания и поддержки.

VolCh 4 сен 2012 в 21:42

Насколько я понял, основные тезисы поста о том, как улучшить код, который непонятен, чужой или, внезапно, свой :)

egorF 5 сен 2012 в 08:02

Методы с «and» в названии — это привет из культуры Objective C.

LionMuzzle 5 сен 2012 в 10:35

Интересно, это исследование было для какого-то конкретного языка?

Nashev 5 сен 2012 в 17:05

ага, brainfuck

bO_oblik 5 сен 2012 в 05:15

Я думаю имеет смысл разделять код на отдельные методы, только если есть необходимость их повторного использования, в ином случае больше минусов, чем плюсов.

mihaild 5 сен 2012 в 10:27

Не согласен. Надо выделять отдельные методы там, где они делают какое-то конкретное отдельное действие. Совершенно неважно, будет ли при этом метод вызываться несколько раз, или нет.
Более того, если (случайно) одинаковые 5 строк кода встречаются в программе дважды — это еще не повод выносить их в отдельный метод, если у них разная семантика.

VolCh 5 сен 2012 в 10:40

Иногда от семантики можно абстрагироваться, например используя приёмы ФП. Хотя в некоторых языках это только ухудшит читаемость.

mihaild 5 сен 2012 в 10:43

Вроде бы основной идеей ФП является то, что программа вычисляет некоторую функцию, которая представляется в виде композиции других функций. И в нормальном коде каждая функция должна иметь некоторый самостоятельный смысл.

VolCh 5 сен 2012 в 10:49

Я ФВП имел в виду, прежде всего. Например при императивной обработке часто встречается выполнение каких-то действий над элементами массива в цикле, в двух местх может быть такой синтаксически идентичный код, но семантический разный. А с помощью ФВП мы может отделить собственно итерирование массива от действий, передавая функцию с действием параметром в функцию-цикл.

mihaild 5 сен 2012 в 14:57

Ну, называть всё, использующее map, функциональным программированием — ИМХО, немного перебор. Хотя, конечно, это элемент оттуда.
Но даже в этом случае ИМХО лучше сделать 2 синтаксически одинаковых функции с разными названиями. Очень вряд ли такие куски кода будут длинными, и нет никакого повода надеяться на то, что и в будущем они будут одинаковы.

VolCh 5 сен 2012 в 17:23

Не только map — reduce, fiter и т. п., а главное более сложные действия.

mihaild 5 сен 2012 в 17:45

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

VolCh 5 сен 2012 в 10:34

Объективный минус — снижение быстродействия и повышение потребления памяти, но обычно пренебрежимое. А так разделение на отдельные функции/процедуры читаемость повышает при условии правильной декомпозиции и хороших имён функций. Конечно если писать в стиле $a1 = func(1); $a2 = func2($a1); то читаемость скорее снизится.

spacediver 5 сен 2012 в 05:54

cleanupEntriesNewerThanLastDownloadedUnixtime — омг, на этом я сломал моск. Поэтому для внутренних имен обычно использую highly_readable_lower_case_with_undercores

ncix 5 сен 2012 в 07:20

cleanupEntriesNewerThanLastDownloadedUnixtime
За таки названия методов извините, надо от ~~церкви~~ компа отлучать.

Stam 5 сен 2012 в 07:30

Это дело вкуса и установленного стиля/гайдлайнов. developer.apple.com/library/ios/#documentation/uikit/reference/UIViewController_Class/Reference/Reference.html

- (BOOL)automaticallyForwardAppearanceAndRotationMethodsToChildViewControllers

Stam 5 сен 2012 в 07:31

PS. Я не говорю, что это шикарное название, но такое имеет место быть.

limitium 5 сен 2012 в 07:23

cleanupEntriesNewerThanLastDownloadedUnixtime
Осталось писать код в названии метода!

egorF 5 сен 2012 в 08:03

См. комментарий прямо сразу над вашим :)

skor 5 сен 2012 в 07:25

Вместо выноса в отдельные функции для первого способа я бы предпочел вынос в переменные:

$is_valid_device = $device->token != "";
$device_expired = $device->expire <= $now;

$is_locatable = $is_valid_device && $device_expired;

return $is_locatable;

Тем более отлаживать это проще и выводить в лог переменные.

egorF 5 сен 2012 в 08:03

А потом где-то забыть что существует такая переменная и забыть ее обновить — и привет, многочасовой дебаг.

НЛО прилетело и опубликовало эту надпись здесь

VolCh 5 сен 2012 в 10:43

Спорное решение, по-моему, хотя лучше, чем оригинал :)

skor 5 сен 2012 в 12:07

Я взял это решение в книге Макконнелла «Совершенный код», оно там предлагалось для упрощения понимания сложных условий.

m_z 5 сен 2012 в 13:20

А чем проще?

У методов есть два плюса:
1) их легче покрыть автоматическими тестами, при необходимости
2) их можно использовать повторно

skor 5 сен 2012 в 13:35

Я говорю про конкретный пример, в котором условия очень простые, что тестами их покрывать даже не нужно. Про второй пункт согласен. Если чуть сложнее или повторно используется, то, конечно, в отдельный метод.

m_z 5 сен 2012 в 18:20

Хе-хе, для вас условия то простые, а тесты автору бы очень помогли. Почему? На первую ошибку уже указал relgames. Вторая ошибка в конечном isDeviceExpired — условие инвертировано неправильно. В исходном примере если текущая дата === expired, то device считается expired. В конечном — еще нет. Мелочь, а поведение после изменения изменилось. Это также подверждение того, что без тестов это не рефакторинг.

egorF 5 сен 2012 в 19:47

Примеры здесь приведены синтетические.

skor 5 сен 2012 в 13:36

То есть, если мы хотим только читаемость, то переменные вполне подходят для простых случаев.

vbif 5 сен 2012 в 08:06

Прошу прощения, что лезу с ламерским вопросом, но мне кажется, что в коде нужно подняться выше и реализовать getOption так, чтобы оставить config, а если нужно разделять единичный и множественный, добавить опцию типа multiple config. Тогда можно будет написать

function getConfigs() {
   if $this->hasOption ('config') {
        $configs = getOption ('config);
        $configs->rootFolder = $this->figureOutRootFolder();        
    }
    return $configs;

Нет?

lalaki 5 сен 2012 в 08:54

«Есть такое правило: выносить в отдельные методы только тот код, который может использоваться где-то еще.»
— да, такие «рассуждения» часто встречаются, причем на вопрос «почему такое правило? почему нельзя выносить даже код, использующийся один раз, если это облегчает читаемость?» ответить обычно не могут.

«Если выносить все методы, то тогда возрастает когнитивная нагрузка из-за того, что программист вынужден будет ездить по файлу вверх-вниз в поисках нужного метода, вместо того, чтобы видеть все перед глазами.»
— для этого есть контраргументы, первый из которых и приведен в статье: вынос как раз снижает нагрузку, потому что 1) гораздо быстрее въезжаешь в верхнеуровневый смысл кода; 2) не всегда нужно смотреть методы нижнего уровня; 3) даже если посмотрел внутренний метод один раз, больше он тебе не нужен и не мешает, а если не выносить в метод, этот код все время будет перед глазами

VolCh 5 сен 2012 в 10:53

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

Это же увеличение исходного кода на N байт, увеличение времени исполнения на M наносекунд и увеличение потребление памяти на L байт!1!1!!!11!!!

Mrrl 5 сен 2012 в 18:02

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

mihaild 5 сен 2012 в 18:41

Если Ваш код тормозит из-за операций вызова функций — то либо напишите его нормально, либо смените компилятор или язык.

Mrrl 5 сен 2012 в 18:48

А еще можно вставить перед функцией inline (если это C++). Или включить оптимизацию. Или посоветовать пользователю купить компьютер пошустрее. А если все это не поможет — заняться другим проектом, где такого быстродействия не нужно.

TheHorse 5 сен 2012 в 21:19

А еще компилятор может забить на inline, и даже на force inline… Остаются только макросы)

simbiod 5 сен 2012 в 09:21

Очень порадовало «figureOutRootFolder».
Жду реализацию с использованием исскуственного интелекта и методами figureOutWhatToDoNextAndDoIt…

Mrrl 5 сен 2012 в 10:03

А зачем тут искусственный интеллект? Такое должен уметь любой make, в сущности, в этом его работа и заключается. Разобраться в ситуации, посмотреть, что еще не сделано, выбрать из этого, что надо сделать в первую очередь — и сделать.

relgames 5 сен 2012 в 10:06

function isOurTokenValid() {
    return ($this->token == "");
}

Должно быть $this->token != "", разве нет?

egorF 5 сен 2012 в 10:07

Верно, спасибо. Исправил.

relgames 5 сен 2012 в 10:09

По теме — абсолютно согласен с автором. Очень сложно читать даже свой код через пару лет, что уж говорить про чужой.

lair 5 сен 2012 в 11:02

function locator($device) {
    // Can we locate the device?
    if ($device->token != "" && $device->expire <= $now) {
        return false;
    }

    $modelNotifier = new ModelNotifier($device); 
    return $modelNotifier->go();
}

Каков тип $device?

А какая разница?

Какой тип у $device->expire и $now?

А какая разница?

Это точно число или там происходит неявное сравнение дат?

А какая разница?

Что возвращает $modelNotifier->go()?

О, добрались до первой реальной проблемы. go(), очевидно, не должен возвращать ничего, потому что из названия метода никак не вытекает, что он должен что-то возвращать.

То же относится и к самой функции locator. Локатор — это объект, а не действие, функция так называться не может.

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

siberiano 5 сен 2012 в 12:32

> cleanupEntriesNewerThanLastDownloadedUnixtime

Мои глаза!!!

egorF 5 сен 2012 в 14:17

Не программировать вам под iOS/Cocoa с такой болью. :))

grigoryvp 5 сен 2012 в 13:02

Вы изобрели SLAP — Single Level of Abstraction Principle :)

BTW, очень хороший принцип программирования. В интернете по этому ключевому слово много статей, обзоров, примеров для разных языков.\

KeFA 5 сен 2012 в 16:18

ничего не понятно
непонятные переменные
огромные методы
неочевидные алгоритмы
хаки, нарушения инкапсуляции
высокая сложность и запутанность
изменения приводят к сюрпризам
ничего не понятно

Вы забыли про дублирование

1nt3g3r 5 сен 2012 в 16:54

«Чистый код», Роберт Мартин. Глава вторая «Содержательные имена». Автор хорошо расписал тему правильного именования, и для чего это нужно.

depp 5 сен 2012 в 17:38

Начало статьи (где говорится про универсальную механическую методику для программистов любой квалификации) противоречит ее же финалу (где уже начинает требоваться мудрость).

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

Я такой код видел пару раз. Заходишь в метод — там if-else ветки и вызов других методов, и больше практически никакого кода нет. Заходишь в эти методы — там то же самое. И так рекурсивно на 5-6 (иногда и 10) уровней вниз. Где-то там после 6 уровня вложенности метод делает что-то типа вашего:
return ($this->token != "");

К этому моменту мы уже имеем дерево из 15-20 методов, которые нам надо держать в голове, чтобы понять что реально происходит в исходном методе, который нам требовалось понять (или поменять).

В общем, не зная меры, такой методикой можно сделать код только хуже.

egorF 5 сен 2012 в 19:49

Любой методикой без вкуса можно сделать хуже. Но в целом да, вы правы, здесь перегибать палку не стоит.

Lipa 5 сен 2012 в 17:51

Когда начал читать статью не ожидал узнать что-либо полезное. Но сейчас уверен, что как только следующий раз сяду за код, начну уделять больше внимания названиям переменных и функций. Спасибо.

vip_delete 5 сен 2012 в 19:44

Я бы написал так:

function locator($device) {
    if ($device->token != "" && $device->expire <= $now)
        return false;
    else
        return new ModelNotifier($device)->go();
}

egorF 5 сен 2012 в 19:50

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

Я запрещаю в своих проектах использовать короткие записи if/for без блоков.