Как говорил Парацельс, всё есть яд и всё есть лекарство — зависит от дозы. Например, не очень опытный разработчик обычно может из простого класса сделать три с использованием метаклассов и прочих, не так часто используемых в реальной жизни конструкций языка. И объяснит это принципом единственной ответственности так, как он его понимает — надо делить код на функции и методы, которые выполняют одну узкую и специфическую работу.
Да, самый очевидный способ упростить жизнь разработчику — это поделить большой кусок кода на несколько небольших, но делающих одну конкретную работу и делающих её хорошо. Это UNIX-way в разработке. Наделять классы, функции, модули, пакеты, сервисы и т. д. единственной ответственностью кажется естественным и понятным. Более того, кажется, что этого достаточно для того, чтобы хорошо декомпозировать код на меньшие части.
Но принцип на то и принцип, чтобы не быть непреложным законом. Нужно чётко понимать границы его применимости и, более того, наивное восприятие принципа единственной ответственности.
Для иллюстрации попробуем взять относительно несложный кусок кода и по-разному его декомпозировать. Для простоты будем считать, что у нас есть единственный инструмент декомпозиции — выделение в отдельную функцию, а фреймворк отсутствует. И постараемся понять, какое разбиение лучше или хуже, какое соответствует принципу единственной ответственности, а какое нет.
Например, у нас есть код контроллера из мифического приложения «Блог». В контроллере стандартная последовательность действий:
берём из сессии пользователя;
получаем данные поста;
проверяем доступы;
проверяем данные из формы;
обновляем данные в базе.
@bp.route('/<int:post_id>/update', methods=('POST'))
def update_post(post_id):
db = get_db()
user_id = session.get('user_id')
# Получаем данные о пользователе из БД
user = db.execute(
'SELECT * FROM user WHERE id = ?', (user_id,)
).fetchone()
# Получаем данные о посте из БД
post = db.execute(
'SELECT p.id, title, body, created, author_id, username'
' FROM post p'
' WHERE p.id = ?',
(post_id,)
).fetchone()
# Если пост не найден, кидаем 404-ю
if post is None:
abort(404, "Post id {0} doesn't exist.".format(id))
# Проверяем, что пост принадлежит автору
if post['author_id'] != user['id']:
abort(403)
# Валидируем заголовок из формы в запросе
title = request.form['title']
if not title:
error = 'Title is required.'
# Валидируем тело поста
body = request.form['body']
if not body:
error = 'Body is required'
# Если всё без ошибок, обновляем данные в базе и редиректим на пост
if error is not None:
flash(error)
else:
db.execute(
'UPDATE post SET title = ?, body = ? WHERE id = ?',
(title, body, id)
)
db.commit()
return redirect(url_for('blog.index'))
# redirect to update
return render_template('blog/update.html', post=post)
Очевидно, что в этом коде слишком многое делается. Читать такой код сложно. И его надо разделить на части.
Попробуем раздекомпозировать кусок кода, который выбирает из базы пост по id и проверяет права доступа:
# Получаем данные о пользователе из БД
user = db.execute(
'SELECT * FROM user WHERE id = ?', (user_id,)
).fetchone()
# Получаем данные о посте из БД
post = db.execute(
'SELECT p.id, title, body, created, author_id, username'
' FROM post p'
' WHERE p.id = ?',
(post_id,)
).fetchone()
# Если пост не найден, кидаем 404-ю
if post is None:
abort(404, "Post id {0} doesn't exist.".format(id))
# Проверяем, что пост принадлежит автору
if post['author_id'] != user['id']:
abort(403)
Что с этим куском можно сделать? Этот кусок кода можно положить в отдельную функцию целиком. А потом, например, отрефакторив, сделать вместо двух запросов в БД один.
def get_post_for_user_id(db, post_id, user_id):
# Получаем данные о посте из БД
post = db.execute(
'SELECT p.id, title, body, created, author_id, username'
' FROM post p'
' WHERE p.id = ? AND p.author_id = ?' ,
(post_id, user_id)
).fetchone()
return post*Немного поменялась семантика, мы уже не отдаём 403 при отсутствии прав, но:
Это может быть даже лучше и безопаснее.
Закроем на это глаза.
Или, наоборот, можем сделать разделение более гранулярным, выделить несколько функций:
def get_user_by_id(db, user_id):
user = db.execute(
'SELECT * FROM user WHERE id = ?', (user_id,)
).fetchone()
return user
def get_post_by_id(db, user_id):
post = db.execute(
'SELECT p.id, title, body, created, author_id, username'
' FROM post p'
' WHERE p.id = ?' ,
(post_id,)
).fetchone()
def can_user_edit_post(user, post):
return user['id'] == post['author_id']
Какой из этих двух вариантов лучше и правильнее?
1-й:
post = get_post_for_user_id(db, post_id, user_id)
if not post:
abort(404)
2-й:
user = get_user_by_id(db, user_id)
post = get_post_by_id(db, post_id)
if not can_user_edit_post(db, user, post):
abort(404)И первый, и второй варианты снижают когнитивную нагрузку на восприятие кода, его становится проще читать. И каких-то причин считать, что один из вариантов с точки зрения читаемости лучше — нет. Семантика «возьми мне пост из базы по id, который принадлежит этому пользователю» — понятная, конкретная задача. Но и «возьми объект», «возьми пользователя», «проверь, может ли пользователь изменять объект» — тоже набор конкретных и понятных задач.
Опять-таки с точки зрения читаемости все три варианта — ок. Нет ни одной веской причины предпочесть хотя бы один из них. Мы можем почти бесконечно сужать, расширять, обобщать, конкретизировать, модифицировать область ответственности любого куска кода. Вариантов разбиения сложного кода, которые снижают когнитивную нагрузку восприятия, — бесконечно много. Но нужно вспомнить, что работа программиста — это изменение кода, а не просто чтение. И то, что код красиво выглядит и хорошо читается, не значит, что в дальнейшем его будет проще править.
Хороший или плохой код определяется тем, легко ли будет в дальнейшем исправить его. Типичный аргумент, который приводится, когда разработчики решают, нужно ли делать так или иначе, звучит как «а что, если?»
«А что, если тебе понадобится использовать вместо Posgresql Mysql или Oracle?»
«А что, если у тебя в списке будет не 5 записей, а 30, 3000?»
«А что, если у тебя поменяется реализация алгоритма?» и т. д.
В общем суть аргумента проста — насколько твой код готов к будущим изменениям? Потому что именно это определяет качество кода — насколько легко его будет править и читать (для того, чтобы потом исправлять).
Часто молодые и горячие программисты в угоду универсальности приносят всё, придумывают невероятные крайние ситуации и под них пишут код, часто готовятся к ситуациям, которые никогда не произойдут. Собственно, и сам Боб Мартин переформулировал принцип единственной ответственности в терминах будущих изменений:
Если посмотреть с этой точки зрения на функциональное разделение кода в нашем примере, ошибки при распределении зон ответственности в случае таких маленьких функций стоят не очень много, да вообще ничего, по сути, не стоят. В конце концов, как бы ни был написан этот код, его слишком мало, чтобы вызвать настоящие проблемы при правке или чтении. Но в случае ошибок при наследовании одного класса от другого или разделении кода на сервисы в рамках сервисной и микросервисной архитектуры, исправить и жить намного тяжелее.
Само по себе наделение какой-либо ответственностью не бывает правильным или неправильным. Нет единственно верного разделения или выделения ответственностей для классов, функций, методов, модулей, пакетов и т. д. Выделяя код в отдельные абстракции кода — в функции, классы, объекты, модули, пакеты, сервисы и т. д., мы проводим границы, которые либо будут способствовать дальнейшим правкам, изменениям и простоте чтения кода, либо нет. Разделение — это плохо, если дальнейшие изменения становятся сложнее, а не проще.
Комментарии (28)
nin-jin
24.12.2021 15:51+1Первый вариант приводит к созданию 100500 копипащенных функций, каждая из которых используется лишь в одном месте. Ориентироваться в них будет сложно. Поддерживать - тем более. А рефакторинг вообще будет адом. В лучшем случае их содержимое будет тем же, что вы написали во втором варианте. Так что лучше посто перенести их содержимое в место их вызова. И не плодить сущности сверх меры. В одной большой функции разбитой на разделы комментариями разобраться проще, чем в куче функций, передающих друг другу управление. А вот выносить лучше то, что может быть переиспользовано в других местах, как у вас написанов во втором варианте. И, конечно, никаких "бесконечно много вариантов" тут нет. Разумные варианты разбиения обычно по пальцам одной руки можно пересчитать.
mvv-rus
24.12.2021 22:40Первый вариант приводит к созданию 100500 копипащенных функций, каждая из которых используется лишь в одном месте.
Для некоторых языков (C#, Java, Delphi, С++ и прочих «объектно-ориентированных») есть ещё одна альтернатива — создание иерархии классов: общая функциональность выносится в базовый класс в виде базовой реализации метода (обычно виртуального), а специфическая — в специфичную для класса реализацию этого метода, либо вызывамый из него служебный виртуальный метод (см. Шаблон Template method). К примеру, у меня в моей недавней статье на Хабре, посвященной решению тестового задания (судя по рейтингу статьи — уже давно никому не интересного, а потому ссылку давать даже не буду) именно так и сделано на C# немалое количество мест, где нужно делать что-то похожее, но таки отличающееся.
Но вот как такое сделать в вашем основном рабочем (если я себе правильно представляю) языке JS — это я без понятия: я JS знаю весьма ограниченно.
mayorovp
25.12.2021 00:15Но вот как такое сделать в вашем основном рабочем (если я себе правильно представляю) языке JS — это я без понятия: я JS знаю весьма ограниченно.
Точно так же. Там такое же ООП как и в других языках, если не ломать его намеренно и не играть в ФП ради ФП.
marshinov
25.12.2021 22:41Ну все-таки не совсем "точно такое же". Прототипы там, extends - это expression и справа можно вызов функции поставить. Вот это вот все
mayorovp
26.12.2021 00:14+1Это — возможности, а вовсе не ограничения, вот я про что.
marshinov
26.12.2021 11:08Но это весьма спорные возможности:) не просто так же все новые версии es и typescript делают из JavaScript подобие Java/C#
mayorovp
26.12.2021 11:17Какая разница, спорные они или нет? Ничто не мешает вам, при желании, просто игнорировать эти возможности и писать привычное ООП.
marshinov
26.12.2021 11:21Ну все-таки начиная с появления ключевого слова class это "почти привычное ООП". Конструкторы ведут себя иначе, это нужно учитывать (потому что прототипы), static-члены весьма спорные, ибо можно просто сделать export func из модуля (как, кстати, обычно и делают). bind'ы тоже останутся много где. Так что same-same but different.
mayorovp
26.12.2021 11:49Вы путаете парадигму, её реализацию и синтаксис. Так-то на JavaScript можно было писать по всем канонам ООП даже до появления, собственно, классов. А уж после появления классов это стало делать точно не сложнее.
Конструкторы ведут себя иначе, это нужно учитывать
Как именно иначе они себя ведут и зачем это учитывать?
mayorovp
25.12.2021 00:17+2Удивительно читать такие слова от вас. Как скоро вы примените этот принцип к своему личному фреймворку?
nin-jin
25.12.2021 00:48Такое ощущение, что вы знаете про этот фреймворк больше меня самого.
mayorovp
25.12.2021 10:52Я знаю про него ровно то, чем вы поделились на Хабре, а вы выдаёте информацию весьма дозировано.
Но я всё равно помню, что для отображения списка однотипных элементов требуется создать отдельный компонент для элемента списка.
nin-jin
25.12.2021 10:59-1Никогда такого не было. Это как раз в Реакте люди страдают созданием отдельных компонент по каждому чиху, чтобы уменьшить ререндеры.
mayorovp
25.12.2021 11:00https://habr.com/ru/post/565414/а не, вру, это не тоВот, нашёл немного: https://habr.com/ru/post/341146/
Но точно помню, что ещё раньше вы кому-то доказывали в комментариях что циклы в шаблонах не нужны.nin-jin
25.12.2021 11:13И где же там написано про создание отдельных компонент?А, вы про то, что все компоненты инстанцируются через автоматически генерируемые локальные фабрики, которые контролируют время их жизни, дают им глобально уникальные идентификаторы, и служат точками расширения.
PavelZubkov
26.12.2021 17:29+1> Но точно помню, что ещё раньше вы кому-то доказывали в комментариях что циклы в шаблонах не нужны.
Конкретно в $mol, нет в "шаблонах" циклов и любой логики.
В привычном понимании шаблон - это что-то похожее на html с вкраплениями каких-то команд(инетрполяция,условия,циклы), служащее для того, чтобы обозначить структуру дом-поддерева/компонент и вложить в него/них данные(и еще что-нибудь, и отколенения в какую-то сторону - у всех свои вариации)
В $mol компонент - это ts-класс, описывается тремя файлами: "шаблон" без логики(view.tree), логика/поведение(view.ts), стилизация(view.css/view.css.ts)
Файлы с поведением/стлизацией опциональны.
Сам "шаблон", больше похож не на шаблон в привычном понимании, а на интерфейс - .т.е. он описывает каким будет класс компонента. А фреймворк из него генерирует ts-класс(view.tree.ts). А пользовтель создает файл с логикой(view.ts) в котором наследуется от сгенерировано класса и добавляет туда поведение
>Я знаю про него ровно то, чем вы поделились на Хабре, а вы выдаёте информацию весьма дозировано.
хотел написать про циклы, а получилась вводная про компоненты) Если интересно могу подробнее.
NurGeo
24.12.2021 17:10+1В одной большой функции разбитой на разделы комментариями...
Читая различные книги, различных авторитетных авторов (например того же дядюшки Боба "Чистый код") встречался с тем что необходимо избегать обильного комментирования кода. Если есть желание кусок кода "обернуть" комментарием то этот код самый верный кандидат на вынесение в отдельный метод или класс. Имя метода (класса) станет абстракцией/черным ящиком которая явно скажет что делает этот кусок кода.
Иначе код будет:
Захламлен обильными комментариями, что затрудняет чтение кода, к тому же хорошие комментарии тоже надо уметь писать.
Необходимо будет следить, чтобы комментарии соответствовали коду при рефакторинге, что дополнительная нагрузка для программиста.
Если профукать предыдущий пункт, то несоответствие комментария и кода создаёт намного большую нагрузку. Намного лучше если не будет комментария, чем если он возможно не будет соответствовать коду.
Методы/функции будут непомерно большими и сложно понимаемыми.
Я лично стараюсь вообще не писать коментарии, стараюсь чтобы код сам рассказал что происходит. Обычно если метод/функция действительно короткая, то в необходимости докстроки нет необходимости. Хотя конечно всегда есть исключения, и комментарии пишутся там, где кажутся уместными.
Автор добавил комментарии, чтобы читатель быстро вник в контекст. Обычная практика при написании статьи. Да и функция большая делающая много чего, что комментарии напрашиваются.
куче функций, передающих друг другу управление
Обычно все таки не нужно знать как метод достигает цели, мы смотрим на него как на черный ящик. Если все таки нужно знать реализацию, то опять же имя метода говорит, что должен сделать метод, код расскажет как он это делает. Опять вижу только удобство;
Первый вариант приводит к созданию 100500 копипащенных функций
обычно когда делишь на меньшие методы/классы/... то больше шансов, что не будет копипаста. Вопрос только в том как правильно декомпозировать. Но похоже мы вернулись к теме поднятой автором).
nin-jin
24.12.2021 17:24+1Хорошее имя функции придумать ещё сложнее хорошего комментария.
Имена функций точно так же может разъезжаться с кодом.
Лучше, если кода будет меньше, чем больше.
Линейный код понимается легче, чем нелинейный.
Комментарий тоже это рассказывает и позволяет перескакивать неинтересные куски.
В контрасте нет ничего плохого.
Danik-ik
24.12.2021 21:43+2Хорошее имя функции придумать ещё сложнее хорошего кокомментария
Если функция делает что-то одно, и программист знает, что именно она делает - не так уж и сложно.
Имена функций точно так же может разъезжаться с кодом
... Но заметить это проще. Сто раз ловил себя на том, что при анализе кода комментарии автоматически фильтруютя и вообще не воспринимаются как смысловая часть.
Лучше, если кода будет меньше, чем больше.
Линейный код понимается легче, чем нелинейный
Согласен. Но удачная декомпозиция позволяет вообще не лезть в детали. Гораздо проще увидеть общую картину. Даже если кода в целом больше, то в фокусе внимания намного меньше, и на каждом уровне обобщения он связан с чем-то одним. И получается более линейным — редко где глубокие вложения остаются.
Комментарий тоже это рассказывает и позволяет перескакивать неинтересные куски.
Даже не заглядывать в неинтересные куски, не искать конец неинтересного куска (потому, что он представлен одной строкой вызова) — всё это признаки удачной декомпозиции.
В контрасте нет ничего плохого.
Хорошее вообще встречается чаще плохого, а правильные решения чаще неправильных (при приложении осознанных и целенаправленных усилий). Главное не доводить до маразма следование любым теориям, и уважать и ценить чужой труд. И любой код перестанет огорчать. Даже тот, который написал я-вчерашний.
Deosis
27.12.2021 10:13Иногда комментарии обясняют, зачем нужен код, так как что и как он делает можно узнать из названия метода и его тела.
fpinger
25.12.2021 03:02А по моему в данном виде не нужно вести рассуждения об единственной ответственности. В данном случае не определена задача. Но всё же порассуждаем. Логически если есть проверка на владельца поста, значит их много. А если много пользователей, то есть администратор/модератор/редактор. Следовательно есть другой код для них и значит единственная ответственность правка поста размазана по нескольким местам)))
Другими словами нам нужно вести речь не о получении пользователя и связанного с ним поста, а с получением редактирующего, поста, проверкой прав на правку данного поста редактирующим и так далее.
GoodGod
25.12.2021 11:30+1Написав метод get_post_for_user_id мы потеряли информацию.
Во-первых метод должен называться get_post_by_post_id_and_user_id. Потому что название get_post_for_user_id больше подходит для get_posts_for_user_id - когда мы достаем все посты по user_id. Еще есть за что зацепится по П.1: у нас в базе нет одной сущности post для user_id, у нас есть массив posts для user_id. Т.е. в базе есть только однозначное соответствие массива постов posts и user_id. И нет однозначного соответствия какого-то одного поста для user_id. Но может быть last_post_for_user_id, most_rated_post_for_user_id, random_post_for_user_id и т.д. В названии get_post_for_user_id теряется однозначность - какой пост мы достаем = теряется информация.
Во-вторых при применении этого метода в проверке прав доступа, при отсутствии поста (пост с post_id не существует) должна быть одна ошибка, а если post.user_id != user_id, то это уже другая ошибка. Если мы и там и там создаем одну и ту же ошибку - информация теряется. Даже если мы наружу отправляем и там и там одну - 404 ошибку, внутренняя ошибка должна быть разная (для логов, для security отдела и т.д.).
mSnus
25.12.2021 22:01+1Серьёзно, в названии метода надо дублировать используемые в нем параметры? Я бы назвал функцию post_for_user()
Tellamonid
25.12.2021 11:41Нет единственно верного разделения или выделения ответственностей для классов, функций, методов, модулей, пакетов и т. д.
Единственного верного нет, но есть предельный случай, до которого, например, доходят в книжке Growing Object-Oriented Software, Guided by Tests (GOOS).
Они разделяют классы/методы до упора, пока они делятся по зонам ответственности. Пользователя, и права пользователя они бы точно разделили.
Я не говорю, что это хорошо, просто слово "единственный" напомнило эту книжку.
tormozz48
26.12.2021 11:12Не силен в Питоне, но кажется здесь в примере кода ошибка:
def get_post_by_id(db, user_id): post = db.execute( 'SELECT p.id, title, body, created, author_id, username' ' FROM post p' ' WHERE p.id = ?' , (post_id,) ).fetchone()Должно быть так:
def get_posts_by_user_id(db, user_id): post = db.execute( 'SELECT p.id, title, body, created, author_id, username' ' FROM post p' ' WHERE p.author_id = ?' , (user_id,) ).fetchone()или так:
def get_post_by_id(db, post_id): post = db.execute( 'SELECT p.id, title, body, created, author_id, username' ' FROM post p' ' WHERE p.id = ?' , (post_id,) ).fetchone()
panzerfaust
Так и в чем же принцип не работает? Вы же таки декомпозировали код, а не решили оставить как есть.
Ну и кроме того, сам SRP изначально как раз и формулируется в терминах связности и причин к изменениям: "There should never be more than one reason for a class to change". А вовсе не в терминах "делать что-то одно и хорошо". Это лишь следствие, которое лучше укрепилось в массовом сознании.
zloy_stas Автор
Да, все так. Статья в большей степени про то, что наивное восприятие принципа SRP приводит к проблемам, особенно у молодого поколения. Так-то сам принцип по разному формулировался Мартином в разное время и начинался с относительного "наивного" определения и пришел по итогу к по сути к принципу loose coupling & high cohesion. И LCHC на мой личный взгляд является более объемлющим и более применимым в разработке на самом разных уровнях.
GoodGod
Я считаю что в идеале код должен быть разделен на минимальные частицы, насколько это возможно.
Но мы можем "схитрить" и упростить код - взять на себя ответственность и объединить некоторые участки вместе, как это сделано в варианте 1 - get_post_for_user_id, заранее предполагая, что этот функционал тоже будет меняться вместе. Т.е. мы упрощаем архитектуру - ради повышения удобочитаемости и когнитивного восприятия кода, под собственное видение программиста каким образом бизнес будет развивать свой проект и с какими задачами на изменение бизнес будет приходить к нам (с какими "осями изменений", "линиями разреза" бизнес будет приходить к нам).