TL;DR: Мы выложили на github наш Code Conventions и рассказали в статье о том, как его применять на практике.
При разработке больших продуктов существует распространенная проблема — с течением времени накапливается много legacy-кода, задачи делаются все медленнее и медленнее. Также при росте команды разработчики начинают писать код по-разному и отсутствие единых правил может приводить к конфликтам и спорам.
За 4 года существования нашего проекта мы сделали больше 20 000 Pull Request’ов (далее PR) и под катом я расскажу, как же мы решили эти проблемы.
Я постарался написать максимально полезную статью. Она будет вам интересна, если вы сталкивались с перечисленными ниже проблемами.
Если вы технический директор или техлид:
- Приходится потратить много времени, чтобы понять, что происходит в незнакомом вам коде проекта
- По каждой задаче вам приходится указывать разработчикам на одни и те же ошибки
- После обсуждения того, как делать фичу, разработчик делает PR не такой, как договорились
- После неправильного PR разработчик, исправляя одни ошибки, делает другие
- Вы тратите большое количество времени на обучение каждого нового разработчика, даже если он потом не задерживается надолго
- Старшие разработчики придумывают свои правила и навязывают их менее опытным ребятам
Если вы product manager:
- При обсуждении каждой (казалось бы не самой сложной) фичи разработчики говорят вам, что ее долго и дорого делать
- Вы примерно понимаете принципы программирования, но в коде вашего проекта вам ничего не понятно
- Вам приходится долго ждать, пока новые разработчики входят в курс дела, прежде чем смогут выполнять сложные задачи
Начало
В начале 2014 года в Roistat работало всего 2-3 разработчика, но первые проблемы начались уже тогда. Мы столкнулись с тем, что каждому разработчику необходимо объяснять одни и те же ошибки. И с ростом количества разработчиков эта ситуация только усугубилась.
Также с опытными разработчиками возникала другая сложность. У них у каждого был свой накопленный опыт и своё понимание, как надо строить архитектуру проекта. Очень часто их мнения противоречили друг другу.
Подобные проблемы накапливались как снежный ком. Было понятно, что если так будет продолжаться, то разработка потеряет скорость, гибкость и каждый релиз будет даваться через боль.
Поиски решения
Сначала мы обсуждали проблемные места и договаривались на словах о том, как будем поступать в таких случаях в будущем. Естественно, это не сработало. Спустя некоторое время у всех было разное представление о том, о чем мы договорились ранее. Также об этих договоренностях не знали новые разработчики.
Тогда мы подумали: «Как же зафиксировать эту информацию для всех в одинаковом виде?». Мы попробовали записывать обучающие видео, в которых рассказывали про архитектуру проекта и основные принципы нашей разработки. Но такой формат оказался очень неудобен. Видео сложно исправлять или перезаписывать. Во время активного роста проекта постоянно возникают новые ситуации и новые проблемы, которые требуют либо новых договоренностей, либо изменения старых.
Затем мы начали записывать итоги наших обсуждений в отдельный документ. На старте это было всего несколько предложений. Но мы договорились, что на Code Review разработчики не должны навязывать своё личное мнение по поводу кода. Вместо этого они должны ссылаться на этот документ и указывать на несоответствия с ним. Если в нём нет нужного правила, то сначала мы должны обсудить проблему, совместно выработать решение, записать его в документ и затем вернуться и продолжить Code Review.
И это сработало. Проблемы с разным пониманием договоренностей стали сводиться к минимуму. В документе мы писали подробные разъяснения с примерами кода, чтобы никто не смог трактовать их по-разному. Каждый новый разработчик в нашей команде сразу мог понять, что от него требуется и как у нас принято поступать в различных ситуациях. Code Review вместо бесконечных обсуждений и споров превратились в довольно прозрачный и прагматичный процесс.
Спустя полгода работы этот документ содержал уже много полезной информации и стал незаменимым инструментом в работе команды. Мы назвали его Code Conventions (или сокращенно Code Conv).
Code Conventions
Code Conv — это правила, которые нужно соблюдать при написании кода. Мы различаем Code Style и Code Conv. Для нас Code Style — это внешний вид кода. То есть расстановка отступов, запятых, скобок и прочего. А Code Conv — это смысловое содержание кода. Правильные алгоритмы действий, правильные по смыслу названия переменных и методов, правильная композиция кода. Соблюдение Code Style легко проверяется автоматикой. А вот проверить соблюдение Code Conv в большинстве случаев может только человек.
Мы придерживаемся следующего подхода при использовании Code Conv.
Для разработчика:
- Разработчик изучает этот документ перед тем, как написать первую строчку кода в нашей команде.
- Разработчик должен понять и осознать написанные правила. Просто выучить этот документ не получится.
- Разработчик строго соблюдает правила из Code Conv при написании кода.
- Разработчик создает PR и сразу сам же его просматривает и проверяет на соблюдение Code Conv
- Разработчик передаёт PR на Code Review и исправляет ошибки и несоответствия Code Conv, на которые ему указывают и которые он упустил сам
Для Code Reviewer’а:
- Ревьювер проверяет код на соответствие правилам, описанным в Code Conv
- Если во время Code Review к разработчику появляется какое-то замечание, то оно должно сопровождаться ссылкой на Code Conv.
- Если в Code Conv нет нужного пункта, то собирается рабочая группа, которая вырабатывает новое правило и фиксирует его в документе. После этого ревьювер возвращается к проверке PR
Немного примеров
Для того, чтобы было понятнее, о чем я говорю, вот несколько примеров, как этот документ выглядит в реальности.
В начале документа есть оглавление. Со временем информации становится всё больше и без оглавления — никуда.
У нас есть 3 основных раздела: ценности, общие принципы и конкретные правила.
Ценности — это требования бизнеса к команде разработки. Бизнес не знает подробностей, какие методологии и принципы используются внутри, но ему важно, чтобы продукт разрабатывался просто, быстро и со временем не усложнялся. Этим ценностям должна следовать вся команда при выработке новых правил и для решения вопросов, которые в правилах не описаны.
Принципы — это способы соблюдения описанных ранее ценностей. Они чуть более детальны, содержат основные методологии разработки и подходы, которыми мы руководствуемся. Опытному разработчику не обязательно помнить наизусть весь документ. Ему достаточно понимать эти принципы, а все остальные правила он может вывести сам из этих принципов в любой момент.
Но всё же ценности и принципы можно интерпретировать по-разному. Чтобы избежать недопонимания, в документе есть конкретные правила и примеры кода, показывающие как можно делать, а как нельзя.
Таких правил очень много и именно они составляют основной объем документа. Благодаря им соблюдается порядок.
На практике применение документа выглядит следующим образом.
Делающий Code Review разработчик оставляет комментарии к участкам кода, в которых он заметил несоблюдение Code Conv. В комментарии он указывает, какое правило нарушено. И опционально, если это не очевидно, добавляет пояснение, почему он так считает.
Например, на этом скриншоте мы видим, что нарушен принцип DRY (Don't Repeat Yourself). Несмотря на то что в этом конкретном PR разработчик написал код без повторов и, казалось бы, использовал его только один раз, он все же создал такой метод, который нельзя использовать в дальнейшем без дополнительной проверки. То есть он создал метод, который заведомо требует повтора кода и логики.
Частые возражения разработчиков
Разработчики часто пытаются протолкнуть под различными предлогами свой код, не соответствующий Code Conv. Они находят самые разные оправдания своим нарушениям, но есть несколько самых частых.
«Но один раз-то можно?»
Разработчики думают, что однократное нарушение Code Conv не принесет никакого вреда. Также иногда они считают, что в каком-то не особо важном месте эти правила не несут ценности или не критичны. В ответ на это мы всегда ссылаемся на теорию разбитых окон: «Если в здании разбито одно стекло и никто его не заменяет, то через некоторое время в этом здании не останется ни одного целого окна». Если мы будем делать исключения из правил, то через полгода мы окажемся в ситуации, когда весь код проекта состоит из подобных нарушений.
«Но ведь нужно очень срочно!»
Разработчики часто говорят, что делать нормально будет долго, а клиент ждет решения проблемы и на бой правку надо выложить как можно быстрее. Но, во-первых, Code Conv нигде не запрещает делать первые итерации с упрощенным кодом. Просто этот код всё равно должен отвечать стандартам качества. Во-вторых, если всё же нужно нарушить Code Conv, то допустимо это делать, если созданная энтропия будет не злокачественной (об этом ниже). Но даже в этом случае, у всех должно быть общее понимание, что следование Code Conv в этой ситуации действительно будет стоить намного дороже.
Несколько слов об энтропии. Энтропия — это количество информации, из которой состоит проект (информационная емкость проекта). Если разработчик пишет код, добавляющий критичную информацию, которую обязан знать каждый разработчик, то такая энтропия является злокачественной. Часто такой код является миной замедленного действия. То есть в данный момент это незнание может ни на что не повлиять, а в будущем из-за него могут быть допущены критические ошибки. Такую энтропию мы называем злокачественной. Если же незнание информации никак не повлияет на работу остальных разработчиков, то такая энтропия не несет вреда, и мы называем её доброкачественной.
Например, если мы делаем отдельный микросервис, который будет слушать какие-то события и складывать куда-то статистику, то этот сервис не вносит злокачественной энтропии. Если про этот сервис не будут знать остальные разработчики, то они не допустят никаких ошибок, он не повлияет на их работу. Если он сломается, то кроме владельца этого сервиса никто не пострадает. Так что для такого сервиса допустимо сделать первую итерацию (как Proof of Concept) с низким качеством кода. А затем, если его надо будет развивать, то нужно будет исправить заложенный технический долг и работать с ним уже по всем правилам. В итоге, Code Conv не запрещает экспериментировать и делать быстрые и легкие концепты, если это не влияет на остальную систему.
Заключение
Такой подход позволил нам уже более четырех лет поддерживать высокий темп разработки, легко вводить в работу новых разработчиков и не замедляться с течением времени.
Из минусов можно отметить, что с таким подходом Code Review проходит дольше обычного. Даже маленькие задачки могут задержаться на несколько дней или дольше из-за не самых критичных ошибок.
Но этот минус перекрывается плюсами, так как на бой не попадает злокачественный код и все «окна» остаются целыми.
К нам неоднократно обращались с просьбами поделиться примером Code Conv.
Мы решили поделиться со всеми и сделать его публичным. Вы можете взять его как есть и использовать сразу. Или можете изменить под себя. Также мы открыты для PR и рассмотрим ваши предложения по улучшению.
Ссылка на наш Code Conv
Мы активно расширяем команду разработки, поэтому если вам близок наш подход, откликайтесь на нашу вакансию или пишите на почту.
Комментарии (40)
ellrion
05.04.2018 18:45позволил нам уже более четырех лет поддерживать высокий темп разработки, легко вводить в работу новых разработчиков и не замедляться с течением времени
и
с таким подходом Code Review проходит дольше обычного. Даже маленькие задачки могут задержаться на несколько дней
чёт какой то диссонанс у меня вызывают эти два предложения.
Так же в начале была озвучена например проблема легаси кода, но как ее решает конвенция непонятно, собственно никак.
flr Автор
05.04.2018 19:20чёт какой то диссонанс у меня вызывают эти два предложения.
Я понимаю, почему это вызывает удивление, но на самом деле всё объяснимо. Когда весь код написан по строгим стандартам, то разработка задачи занимает всегда понятное и предсказуемое время. Например, написание кода для условной задачи может занять неделю, а прохождение PR пару дней. Если же код не придерживается никаким правилам, то PR может пройдёт за пару часов, вот только разработка может занять 2 недели или дольше. В итоге PR проходит дольше, но «Time-To-Market» у задач намного меньше.
Так же в начале была озвучена например проблема легаси кода, но как ее решает конвенция непонятно, собственно никак.
Обычно слово «legacy» несет негативную окраску и по сути означает «дурно пахнущий код». По крайней мере в статье оно применено именно в таком смысле. Четкие правила и стандарты помогают, если не избавиться полностью, то свести к минимуму процент такого кода.
Nelin
05.04.2018 22:44Нельзя нескольким переменным присваивать одно и то же значение
Нельзя изменять переменные, которые передаются в метод на вход
Судя по примерам может быть ситуация когда эти рекомендации… не согласуются друг с другом.
Как разруливаются такие случаи?flr Автор
05.04.2018 22:45А можешь написать пример/псевдокод, о каком случае речь?
Nelin
05.04.2018 23:37function loadUsers(array $ids) { $usersIds = $ids; // ... unset($usersIds[0]); // ... }
Возможно что-то вроде этого.
Конкретно это можно реализовать через array_filter, но все-таки.flr Автор
05.04.2018 23:52Да, ты правильно заметил, что это надо реализовывать через фильтр. Это будет и логичнее, и понятнее, и надёжнее. Описанный выше вариант с unset — это не наш путь.)
Nelin
05.04.2018 23:38Нельзя подключать несколько классов из одного неймспейса через use
Можно развернуть причины такой рекомендации?flr Автор
06.04.2018 00:08Да, попробую кратко.
В большом проекте очень много классов с одинаковыми названиями, но с разными неймспейсами. Если разработчик в коде видит условное new User(), то он не может точно быть уверен, что за User тут имеется в виду. Может Service\Repository\User. Или Entity\User. Разработчику приходится переходить к началу файла и обратно. Это затрудняет чтение кода. Гораздо удобнее читать new Entity\User().
При этом подключение неймспейса через use вместо класса не добавляет никаких сложностей и абсолютно бесплатно. Даже наоборот — меньше мусорного кода вначале файла. То есть мы «забесплатно» можем упростить человеку понимание кода. Не вижу причин не сделать этого.
Видишь ли ты какие-то причины подключать именно класс и прятать неймспейс?Nelin
06.04.2018 00:13Это скорее относится к Следует избегать использования alias.
А в вопросе говориться про разные классы в одном неймспейсе
use Entity\User; use Entity\Project; $user = new User(); $project = new Project();flr Автор
06.04.2018 00:22Здесь похожая ситуация и та же причина. Просто я пример неудачный написал. Достаточно мой пример заменить на твой, и ответ будет тот же. В коде мы видим new User() и new Project(). Но мы не знаем, что это за классы, так как их неймспейсы где-то вверху файла. Это в четырех строчках всё понятно, а классы с логикой обычно на экран не помещаются.
Fesor
06.04.2018 10:39на самом деле сомнительное правило… оно мне больше говорит о каких-то проблемах с зависимостями и разделением системы на модули (нэймспейсы тоже модули).
movetz
06.04.2018 00:22Извините, но вы сами себе противоречите:
Модель — простой объект со свойствами, не содержащий бизнес-логики
Должен стремиться к соблюдению принципов GRASP, ...
Так как ваши модели полностью нарушают первый и базовый принцип — Information Expert, а логика в вынесенная в сервисы приводи к увеличению Coupling и уменьшению Cohesion.flr Автор
06.04.2018 00:32Сначала хотел написать, что (для меня) значат в данном случае Coupling и Cohesion, но понял, что это скорее потянет на отдельную статью. Так что на такую абстрактную формулировку могу ответить только так же абстрактно: нет, с моей точки зрения именно такой подход соблюдает GRASP, а не нарушает его. И наоборот — если смешать логику и данные, то GRASP будет нарушен (как и многие другие принципы).
Но как раз для таких случаев у нас не только написано требование соблюдения GRASP, а есть подробные правила. Они расшифровывают этот и другие принципы и описывают, как их понимаем мы. В итоге документ создает единый стандарт. И добавлю, что на какую-то абсолютную истину мы не претендуем. Просто делимся опытом.)Fesor
06.04.2018 10:43с моей точки зрения именно такой подход соблюдает GRASP
Очень сильно зависит от того как используется "модель". Если эти тупые структуры данных используются внутри оболочки какой-то, то в целом я склонен с вами согласится (хотя эта оболочка и будет в этом случае являться моделью, хотя слово вообще бесполезное).
Но обычно оно используется в сервисах-менеджерах, которые имеют зависимость не только от "моделек" но еще и от репозиториев и прочих гейтвеях предоставляющих доступ к данным.
Собственно очень простой способ убедиться есть проблема или ее нет — вглянуть на юнит тесты бизнес логики (и убедиться что они именно юнит а не интеграционные).
movetz
06.04.2018 11:25Хочу уточнить, как я понял, Вы считаете, что такой подход:
class RestrictionService { //... public function blockUser(int $id, string $reason): void { $user = $this->repository->get($id); if ($user->isBlocked) { throw new UserWasAlreadyBlockedException(); } $user->isBlocked = true; $user->blockedAt = \new DateTime(); //more magic ... $user->reason = $reason; $user->blockedCounter++; } }
лучше чем:
$user->block($reason); class User { public function block(string $reason): void { if ($this->isBlocked) { throw new UserWasAlreadyBlockedException(); } $this->isBlocked = true; $this->blockedAt = \new DateTime(); //more magic ... $this->reason = $reason; $this->blockedCounter++; } }
+ второго подхода:
- Не нарушается инкапсуляция, в первом случае кто-то может заблокировать пользователя в обход сервиса, или же написать отдельную «похожую» логику в другом месте и при след изменениях вы получите баг, что где-то блокировка работает не так, а во втором вы четко контролируете процесс изменения состояния блокировок через определенное поведение.
- Мне, как новому разработчику, проще разобраться, так как контракт описан в одном месте и не размазан по cервисам всего проекта.
- Проще писать юнит тесты. Я надеюсь вы их пишете. У меня когда-то был проект, где в тестах мокали с десяток сервисов — не самое лучше занятие разгребать dependency hell, где один сервис вызывает другой, а тот третий и пятый, и так далее.
- Первый подход больше близок к процедурному, нежели к объектному ориентированному — вы можете спокойно заменить ваши модели на массивы, а сервисы на функции (процедуры).
В наших проектах мы пишем логику и там и там, мы пишем юнит тесты на наши сущности и на сервисы, и стараемся последних писать как можно меньше и использовать больше для оркестрации действий.
alexkbs
06.04.2018 13:24В этой части рекомендации содержат противоречие. С одной стороны рекомендуется следовать GRASP, а это как вы сделали — нет вопросов, всё понятно и логично, а с другой стороны требуется чтобы в моделях не было логики, что явно противоречит GRASP. Как ни крути.
В целом есть ощущение что наши уважаемые коллеги поспешили с публикацией черновика своих Code Conventions.
flr Автор
06.04.2018 15:14требуется чтобы в моделях не было логики, что явно противоречит GRASP. Как ни крути.
Явно противоречит… Как ни крути… Я так понимаю тут уже всё понятно и мне нет смысла пытаться объяснять, что каждый может интерпретировать слова Ответственность должна быть назначена тому, кто владеет максимумом необходимой информации для исполнения совершенно по-разному?
Интерпретация по ссылке мне понятна, я понимаю как рассуждает автор, но мне она не близка. Я не настолько экспертен, чтобы говорить, что моя позиция единственно правильная «как ни крути», но всё же приведу ее ниже.
Модель является экспертом только в своих данных и манипуляциями с ними должна заниматься только она. Однако модель не является экспертом в других областях: она не может вызывать чужую бизнес-логику и даже не является экспертом в том, как сохранять себя в базу данных. То есть модель содержит сеттеры, которые правильно изменяют ее состояние. Но эти сеттеры не сохраняют модель в бд, они не вызывают цепочку зависимых сервисов и т.п. Чтобы сохранить модель в бд, не требуется никаких особых знаний. Грубо говоря, все модели сохраняются одинаково. Это довольно абстрактные вещи, чтобы их вот так объяснить в комментарии, но основная суть моей позиции должна быть понятна.
В целом есть ощущение что наши уважаемые коллеги поспешили с публикацией черновика своих Code Conventions.
Спасибо за критику. Для нас каждый отзыв важен.alexkbs
06.04.2018 16:00Вы или ваш коллега прямо пишет что "y объектов с данными не может быть практически ничего кроме геттеров". Эта фраза исключает какую-либо другую интерпретацию кроме полного отказа от шаблона Information expert, который является частью GRASP. Вопросы?
flr Автор
07.04.2018 16:00Вы или ваш коллега прямо пишет что «y объектов с данными не может быть практически ничего кроме геттеров».
По приведенной ссылке общение идёт вообще по другому вопросу (там речь о том, что нужно опускать слово get у геттеров). В коде «$user->registrationDate()» метод «registrationDate()» не может быть ничем иным, кроме как геттером. То есть фраза вырвана из контекста.
Но в любом случае спасибо за отзыв. Это всегда полезно.alexkbs
07.04.2018 16:11В самом документе говорится то же самое:
Модель — простой объект со свойствами, не содержащий бизнес-логики.
Эта требование исключает какую-либо другую интерпретацию кроме полного отказа от шаблона Information expert, который является частью GRASP.
flr Автор
07.04.2018 16:29Спасибо, там действительно было двусмысленно написано. У нас глаз на такие вещи замылен. То что для нас очевидно, совсем не очевидно внешнему читателю. Поправил описание модели. Не идеально, но хотя бы пока что людей не смущать. Если будут варианты лучше — предлагайте.
Fesor
06.04.2018 16:22что каждый может интерпретировать слова… совершенно по-разному?
То есть берем любой принцип и крутим и вертим им как хотим? Нет, это так не работает.
Суть Information expert в том что бы повысить кохижен и снизить каплинг. Если вы сможете мне продемонстрировать вариант при котором все согласно вашему кодстайлу и при этом все согласуется GRASP, было бы славно. И да, такой вариант более чем возможен, просто встречается в природе крайне редко.
она не может вызывать чужую бизнес-логику
потому выделяют application layer который этим заведует.
Но эти сеттеры не сохраняют модель в бд
а это тут причем?
Грубо говоря, все модели сохраняются одинаково.
Я потерял нить… вопервых они не всегда сохраняются одинаково. Во вторых, как вы выше отметили за сохранение отвечают всякие там репозитории, тэйбл гейтвеи и прочие штуки. А потому в контексте "где должна быть бизнес логика" это вообще пустой разговор.
Это довольно абстрактные вещи, чтобы их вот так объяснить в комментарии
предоставьте пример какой-либо простой фичи. Ну или сами предложите какой-либо юзкейс на котором мы сможем разобрать вопрос, а я реализую его в различных вариантах и подходах.
flr Автор
06.04.2018 15:02Спасибо за более развернутый вопрос.
Хочу уточнить, как я понял, Вы считаете, что такой подход:… лучше чем: ...
Не совсем.
Во втором подходе в объекте User метод block() не содержит никакой другой логики, кроме как управления внутренним состоянием объекта. Для нас сеттеры и геттеры — это не обязательно отдать какое-то одно свойство или записать его, не делая никаких других операций с другими свойствами. Геттеры могут получать любые данные из внутреннего состояния объекта, а сеттеры — делать любые нужные манипуляции с ним. Это вскользь упоминается в Code Conv, но возможно стоит как-то более явно это написать. То есть второй вариант не нарушает нашего правила разделения логики.
Но, если мы во втором подходе вместо «more magic» добавим зависимость от каких-то внешних компонентов и внешней бизнес-логики, то это перестанет быть простым сеттером и станет тоже полноценной бизнес-логикой проекта. И тогда для нас это будет уже смешением данных и логики и нарушением Code Conv.
в первом случае кто-то может заблокировать пользователя в обход сервиса, или же написать отдельную «похожую» логику в другом месте
Я не очень понял. Никто не мешает человеку написать свой метод или вызывать изменение параметров напрямую в обход метода как в первом варианте, так и во втором. Это уже вопрос подхода. Если у вас логика лежит с данными, то люди ищут метод там. Если логика лежит в сервисах, то люди ищут метод в них. Когда человек захочет добавить новый метод, он попадет в то место, где уже кто-то добавил метод и не станет добавлять свой дублирующий. Главное, чтобы это место всегда было чётко определено.
В обоих кейсах на самом деле есть проблема и неполноценное следование тому же GRASP. Например, во втором кейсе логика блокировки полностью завязана на объект User, в который эту логику вставили. То есть я не могу создать свой объект MyUser и воспользоваться существующей логикой. Логика строго превязана к конкретному объекту вместо того, чтобы работать с нужным ей интерфейсом и не требовать конкретной реализации. То есть данный код страдает «high coupling» из GRASP. В том числе это за собой тянет нарушение SRP и других принципов.Fesor
06.04.2018 16:33И тогда для нас это будет уже смешением данных и логики и нарушением Code Conv.
то есть проблема в терминологии… вы сеттером называется практически любой метод изменяющий стэйт объекта. Я правильно понимаю?
Логика строго превязана к конкретному объекту вместо того, чтобы работать с нужным ей интерфейсом и не требовать конкретной реализации.
А вот тут есть простор для интерпритации принципов, в силу того что на них влияют ограничения вашего языка программирования. По сути это ограничение классовой модели объектов. Если бы рассматривали язык который наследует объектную модель не от simula а что-то более "объектно ориентированное", то там все уже будет чуть по другому.
Если я вас правильно понял, то вы хотите иметь возможность подменять поведение вокруг одной и той же модели данных. Так?
То есть данный код страдает «high coupling» из GRASP.
нет, так как это один модуль. Внутри модуля высокая связанность между элементами модуля это нормально. Это про ту часть которая high cohesion.
В том числе это за собой тянет нарушение SRP и других принципов.
все упирается в то как вы дробите логику. Если мы возьмем скажем все что связано с юзером и запихнем это в одну сущность мы действительно можем получить нарушение SRP и т.д. но логика внутри сущности может вообще не пересекаться и может быть разделена на отдельные объекты.
michael_vostrikov
08.04.2018 06:32Допустим, есть некоторая внутренняя система. В ней есть действие «создать пользователя», ну или скажем «создать аккаунт пользователя», запускается оператором. То есть на входе форма с данными, на выходе новый объект User. После создания надо отправить письмо на указанную почту. Как правильно организовать архитектуру с соблюдением принципов ООП?
GHostly_FOX
06.04.2018 02:03Нельзя писать глагол get в геттерах
Но при это у Вас по документу только и встречаются в примерах «Хорошо» именно использование геттеров с get. Как-то получается противоречие.
Например:
Сервисы
Вместо лишней конкатенации используем подстановку переменных в двойных кавычках
flr Автор
06.04.2018 02:24Вместо лишней конкатенации используем подстановку переменных в двойных кавычках
Спасибо, поправил. Там была речь про кавычки, так что никто не обращал внимание на геттер.)
Сервисы
А тут, кстати, не совсем то. У сервиса нет геттеров по определению. У сервиса просто методы, которые могут начинаться на get, но это не геттеры. Геттер в нашем документе — это метод, который возвращает какие-то данные из состояния объекта. А у сервисов состояний нет. Их методы, начинающиеся get, просто получают откуда-то какие-то данные.
dumistoklus
06.04.2018 02:04Если дата должна быть представлена скалярным значением, необходимо использовать строку
Что не так с time()?flr Автор
06.04.2018 02:30В целом с time() всё в порядке. Но есть приоритет. Для нас строка лучше числа. Просто удобнее дебажить, выводить пользователю, делать substr для выборки года/месяца/дня и т.д. В общем, удобнее в повседневном использовании. Но, если всё же нужно число, то используем его (правило это не запрещает). Просто на это требуется какая-то причина.
alexkbs
06.04.2018 05:51Со хранением даты и времени в UTC вы рискуете наступить на грабли с временными зонами, которые поменялись задним числом, или поменяются в будущем. Например, человек просит чтобы его в девять утра разбудили через год. Вы посчитали сколько это будет по UTC сейчас и записали, а за год временная зона поменялась и вы разбудили человека не в девять, а в десять. Так себе хорошая идея требовать всегда хранить время по UTC.
Правильно хранить всегда дату и время с временной зоной. Да, программистам будет сложней, но так вы точно никаких концов не потеряете.
flr Автор
06.04.2018 12:03Я возможно не совсем правильно понял, о какой проблеме идёт речь.
Пользователь попросил напомнить ему о чем-то 1 июня 2018 года в 12:00 по Москве (UTC+03). Это определенная точка во времени, когда ему надо напомнить о событии. Если юзер переехал в другое место, то не факт, что точка должна смещаться за ним. Мы по-прежнему напомнить ему должны в 12:00 по Мск. А 12:00 UTC+03 навсегда останется одинаковым и будет равнозначно 09:00 UTC0. Если же в задаче описано так, что при перемещении пользователя данное событие должно тоже двигаться, то хранение зоны в БД нам никак не поможет, нам надо всё равно писать логику, обрабатывающую этот процесс перемещения.
Можешь подробнее написать какой-то пример конкретной задачи?alexkbs
06.04.2018 12:06Хороший пример с Москвой. Пользователь никуда не уезжал из Москвы, но вы записали в БД время 12 часов дня как 09:00 UTC0. Спустя полгода в РФ снова ввели зимнее время, и вот уже ваши 09:00 UTC0 не соответствуют 12 часам дня.
Всё гораздо интересней когда зоны меняются задним числом. Африканские страны всякие интересные есть.
flr Автор
06.04.2018 12:46Да, бывает такое, что временная зона съезжает, как было в РФ. Но, опять же, проблема будет только в том случае, если человеку надо напомнить о чем-то именно в какой-то день единоразово и в какой-то момент времени суток и эта точка времени должна сдвигаться за сменой зоны (хоть пользователем, хоть правительством). У нас просто таких кейсов нет. А даже если появится, то Code Conv не запрещает сохранить в конкретном кейсе в нужном тебе формате. Это как в КОАП — обстоятельство непреодолимой силы. Если надо в какой-то задаче сохранить дату особым способом, то значит надо. А по умолчанию для 99.99% дат — нет.
Но спасибо за уточнение. Интересная мысль, которую обязательно будем иметь в виду.alexkbs
06.04.2018 12:49Дело в том что вы пишите инструкции для кода с заделом на будущее, "for long term projects", а на такие известные и понятные кейсы не рассчитываете. Это противоречие. Вам бы или убрать приставку "for long term projects", или перейти на хранение времени вместе с называнием временной зоны.
Например, вы можете сейчас сохранить время в UTC, а через десять лет потребуется узнать в какой же исходной временной зоне было это время. Что вы будете делать, искать программиста который это писал, менеджера, который время в БД заводил?.. Не для того люди поддерживают базу часовых поясов чтобы вы вот так просто на неё плюнули и побежали изобретать свой очередной велосипед потому что вам неудобно или что. Мир часовых поясов действительно неудобен, нелогичен, далёк от человека. Такой есть, это необходимо учитывать в проектах с расчётом на десятилетия.
flr Автор
06.04.2018 15:28Возможно я неправильно описал, как это происходит у нас. Мы знаем, в какой зоне была сохранена. У пользователя в профиле установлена зона и мы с ней и работаем. То есть мы не «отбрасываем» зону, мы знаем какие зоны у пользователя были в какое время. Просто мы не дублируем её в каждой колонке с датой в каждой записи в БД. Никаких последствий в долгосрочной перспективе я пока не увидел. Может, я что-то недопонял. :<
alexkbs
06.04.2018 15:54Вы пытаетесь найти аргументы почему ваш подход — правильный, но при этом упускаете общую картину. Например, а что если это не один пользователь, а какая-то другая сущность, у которой нет своей временной зоны, которая присуща только ей? Вы тоже будете выдумывать какие-то способы решить проблему, там, сохраняя временную зону в соседней колонке? Согласитесь, странно!
Если сохранять время вместе с временной зоной, для чего есть все необходимые технические средства в PHP и большинстве других языков программирования, то не нужно будет ничего делать специально кроме этого. Не нужно будет отдельно записывать временную зону, не нужно будет отдельно вычислять как изменилось время в моменте относительно данных пользователя, ничего такого делать не нужно будет.
flr Автор
07.04.2018 16:14Я полностью согласен с тем, что это важный вопрос, и я также заинтересован в том, чтобы Code Conv улучшался. Это одна из целей, зачем он выложен в Open Source.
Спасибо за развернутое пояснение своей позиции. Мы это обдумаем и возможно сделаем какие-то доработки в правиле.
AlexLeonov
07.04.2018 18:54Нашел время почитать внимательнее. Нашел еще ряд несуразностей.
Статические вызовы можно делать только у самого класса. У экземпляра можно обращаться только к его свойствам и методам.
Плохо:
$type = $user::TYPE;
Ну вообще-то это не то, о чем вы пишете, а получение константы класса, имя которого в переменной $user. То есть string $user. И ничего криминального в такой конструкции нет.
Код должен быть таким, чтобы его можно было автоматически отрефакторить в IDE (например, Find usages и Rename в PHPStorm). То есть должен быть слинкован типизацией и PHPDoc'ами
В общем случае комментарии запрещены
Желание добавить комментарий — признак плохо читаемого кода.
Ну тут можно только словами анекдота ответить — или трусы снимите, или крестик наденьте…
Читающий не должен держать что-то в уме, возвращаться назад и интерпретировать код иначе. Например, надо избегать обратных циклов do {} while ();
Вы действительно понимаете разницу между циклами с предусловием и постусловием? Это не «возврат назад», а условие, которое впервые будет проверено после первой итерации. Именно поэтому оно и пишется после итерации.
Очень странный пункт, очень.
Модель — простой объект со свойствами, не содержащий никакой другой бизнес-логики, кроме геттеров и сеттеров.
Вы случайно с DTO не путаете? «Модель» в смысле ActiveRecord, например, вполне себе содержит логику.
Желательно делать модели неизменяемыми, см. Работа с объектами.
Нет логики, иммутабельность… Value Objects, да?
API-объект
— Скажите, как его зовут?
— D, парам-пам-парам-пам!
— T, парам-пам-парам-пам!
— O, парам-пам-парам-пам!
— D-T-O!
Файлы классов должны быть расположены в директориях в соответствии со стандартом PSR-0
Deprecated
К переменной нельзя обращаться по ссылке (через &)
Вот так вот взяли и снесли курочке яичко… Не вижу никаких обоснований для неиспользования ссылок. Ну кроме одного — вы не понимаете, что это такое.
Нельзя сортировать ассоциативные массивы
Нельзя смешивать в массиве строковые и числовые ключи
Для проверки наличия значения по индексу в обычных (не ассоциативных) массивах используем count($array) > N
Еще кучу яиц снесли. Второе еще можно понять, первое понять очень сложно, третье — полная жесть. С чего вы взяли, что в PHP есть «обычные» и «ассоциативные» массивы?
В PHPDoc в возвращаемом значении не надо указывать void и null
Полное отсутствие логического обоснования. А если действительно возвращается null или void?
Все методы класса по умолчанию должны быть private
Унаследовал, хочешь перекрыть — попроси разрешения тимлида? Зачем?
Запрещается кешировать данные в статических переменных метода
Причина, видимо, всё та же — не понимаете, что это такое…
В шаблонах не должны вызываться методы объектов
Вообще любимое место…
В целом-то норм, видно, что большая работа проделана. Но что-то мне говорит, что это не действующие правила, а скорее пожелания к коду. Слишком много правил, из которых в реальности постоянно придется делать исключения.
AlexLeonov
Не пробовали, коллеги, встраивать в рабочий процесс PHP Code Sniffer? Мне в свое время удавалось встроить множество правил наподобие «не присваивать под if» в него — и задачи автоматически возвращались разработчику с подробным комментарием.
flr Автор
Правильная мысль. Мы активно используем Code Sniffer и у нас в нём написано довольно много проверок. Они отрабатывают на Pre-Receive Hook, не давая запушить код с нарушениями. Однако поддерживать эти проверки в актуальном состоянии довольно проблемно (кто писал правила под CS, тот поймет). Так что пока некоторые вещи дешевле внести в текстовый документ и не тратить время. Мы всё ждем, когда же появится какой-то аналог CS, но для людей. Чтобы правила можно было делать без слёз и боли. :<