Comments 19
Однако спасибо за перевод, а автору за статью. Я джун, с реальным опытом всего 2 месяца. И, черт возьми, первый пример — это про меня. Но я одумался и буквально в начале недели отрефакторил свой код правильно. Но этот текст помог объяснить самому себе, почему мне первый код не нравился.
С разницей в день вышла другая статья, где была рекомендация, как мне показалось, на похожий случай:
ИМХО:
ИМХО писать 2 функции вместо одной не всегда удобно. Когда, нпр., выбор между коротким или длинным сообщениями отладка и модификация двух функций м.б. заметно сложнее, чем одной.
Не используйте булевы значения в качестве параметров
При разработке функции может возникнуть соблазн добавить флаг. Не делайте этого.
Поясню на примере: допустим, у вас есть система передачи сообщений, и есть функция getUserMessages, возвращающая пользователю все сообщения. Но есть ситуация, в которой вам нужно возвращать либо краткую суть каждого сообщения (допустим, первый абзац), либо сообщение целиком. Поэтому вы добавляете параметр в виде флага или булева значения, который называете retrieveFullMessage.
Повторюсь, не делайте этого.
Потому что те, кто будут читать ваш код, увидят getUserMessage(userId, true) и будут недоумевать, что это вообще такое?
ИМХО:
здесь ИМХО нужен особый подход.
getUserMessage(userId, true) очевидно плохо.
Нужны константы с осмысленными именами:
const retrieveFullMessageFl = true; retrieveShortMessageFl = false;
И вызов записывать только с этими константами:
getUserMessage(userId, retrieveFullMessageFl); getUserMessage(userId, retrieveShortMessageFl);
ИМХО писать 2 функции вместо одной не всегда удобно. Когда, нпр., выбор между коротким или длинным сообщениями отладка и модификация двух функций м.б. заметно сложнее, чем одной.
Вы не контролируете все вызовы вашей функции. Другой разработчик может написать вызов с литеральным значением, мотивируя, например, тем, что оно используется только в одном месте.
В добавок, написание таких дополнительных констант полностью нивелирует выигрыш от неразделения функций: приходится писать кода больше линейно по количеству вызовов.
Кроме того, когда появится требование иногда возвращать только аннотацию краткой сути сообшения (допустим, первые 72 символа), этот код всё равно рефакторить. И в каком-нибудь динамически слабо неявно типизируемом языке с редакторами кода не так здорово, чтобы за три секунды это решить.
Судя по тому, что вы привели, функция остаётся всё-таки одна, просто она управляется не флагом или напрямую булевыми занчениями, а именнованными булевыми константами, а сама функция при этом вообще не изменяется.
Так что, речи об отдельной функции для коротких сообщений не идёт.
Так что, речи об отдельной функции для коротких сообщений не идёт.
Не идет (см. ниже).
В статье про SRP я показывал как это делается. Пока вся реализация находится внутри этого метода, копипастить функцию может быть неудобно.
Однако если ее декомпозировать, то эти две функции начнут выглядеть примерно так.
Нетрудно представить как будет выглядеть второй метод при этом.
Однако если ее декомпозировать, то эти две функции начнут выглядеть примерно так.
getShortMessage(id) {
const data = loadMessageData(id);
return composeShortMessage(data);
}
Нетрудно представить как будет выглядеть второй метод при этом.
Вы предлагаете в любом случае загружать полный объем данных, а потом, если нужно, их сокращать для вывода. Но м.б. такая ситуация: в сокрашенном виде интересует только общая инфа о файле: название, дата создания, размер и т.д., а в полном виде еще файл нужно загрузить, разархивировать, расшифровать и извлечь из него спец. инфу. Это может занять ощутимое время и потребовать значительных ресурсов. Поэтому, когда не нужно, это лучше не делать. Но универсального рецепта на все случаи ИМХО нет.
Я не предлагаю загружать избыточные данные, я предлагаю декомпозировать код, после того как он «нашинкуется» на методы, от избыточная сложности избавиться будет легче. Для флага передаваемого хардкодом не надо придумывать енумы и константы. Его не должно быть. Да, сидеть и заниматься целенаправленно рефакторингом какого то говнометода может быть долго, неинтересно и неоправданно. Надо постепенно расти, стремиться к тому, чтобы изначально не писать таких методов.
ЗЫ и да, в той упомянутой статье есть такой пункт — не надо бояться выкинуть свой код в помойку. Конечно, у нас всегда не хватает времени, жмут оценки, давит начальство, «все было против нашей сборной по футболу»(с)
ЗЫ и да, в той упомянутой статье есть такой пункт — не надо бояться выкинуть свой код в помойку. Конечно, у нас всегда не хватает времени, жмут оценки, давит начальство, «все было против нашей сборной по футболу»(с)
Видимо я неправильно понял Ваш код. Чтобы получить полное сообщение достаточно вызвать loadMessageData, а composeShortMessage выбирает из data только то, что нужно для краткого сообщения.
Это псевдокод, чтобы обозначить то, как примерно должны выглядеть методы.
Мне трудно это все доносить, люди, которые пишут код примерно так как здесь,
Я же предполагаю, что код надо писать примерно так и хардкодные параметры, ака флаги, из такого кода испаряются сами собой.
Это примеры кода из реального проекта. их не предполагается внимательно читать, просто взгядом оценить, чтобы понять о чем идет речь.
Второй пример кода лучше не только потому, что он больше декомпозирован, но и еще потому, что получает список сущностей про списку ключей, а не как оригинальный, который получает всегда по одному ключу, что есть проблема N+1. Речь не о ней, конечно же, но стоило упомянуть, что код который лучше не обязан работать медленнее, он может и быстрее работать, чем «то что было раньше».
PS не получилось картинки из г-драйва подключить, работают если в отдельном окне открыть по правой кнопке
Мне трудно это все доносить, люди, которые пишут код примерно так как здесь,
1
с трудом могут представить как это не делать код с флагом, а то и с двумя-тремяЯ же предполагаю, что код надо писать примерно так и хардкодные параметры, ака флаги, из такого кода испаряются сами собой.
2
Это примеры кода из реального проекта. их не предполагается внимательно читать, просто взгядом оценить, чтобы понять о чем идет речь.
Второй пример кода лучше не только потому, что он больше декомпозирован, но и еще потому, что получает список сущностей про списку ключей, а не как оригинальный, который получает всегда по одному ключу, что есть проблема N+1. Речь не о ней, конечно же, но стоило упомянуть, что код который лучше не обязан работать медленнее, он может и быстрее работать, чем «то что было раньше».
PS не получилось картинки из г-драйва подключить, работают если в отдельном окне открыть по правой кнопке
—
del
То есть вы предлагаете вместо
element.classList.toggle( 'visible' , isVisible )
Везде писать
if( isVisible ) element.classList.add( 'visible' )
else element.classList.remove( 'visible' )Я правильно понял вашу мысль?
element.classList[ isVisible ? 'add' : 'remove' ]( 'visible' )Если я правильно понял посыл автора, то система должна содержать две специализированные функции (add и remove) и, опционально, собранную с параметром (toggle). В первом и особенно во втором примерах обе ветки if-а заинлайнены, а не выделены отдельными именованными функциями.
Sign up to leave a comment.
Избыточная сложность