@yabivipil

А можете поревьювить?

Решил написать свой велосипед (клиент) для телеграма. Там реализован еще не весь функционал, но хочется какой-то конструктивной критики. Можете помочь с этим?

ссылка на репу
  • Вопрос задан
  • 691 просмотр
Пригласить эксперта
Ответы на вопрос 2
gzhegow
@gzhegow
aka "ОбнимиБизнесмена"
Глаза радуются, когда такой код видишь, не надо спрашивать - где он тот кто писал

Немного косметики:
1. Константы можно определять в интерфейсах, это здорово помогает, незачем для них целая папка с классами

2. Билдер предполагает (на мой взгляд) строительство чего-то сложного в такой нотации
->where()->where()->group(). Если нечто просто возвращает другой объект, это может быть фабрикой. Я имею в виду даже называться фабрика. Билдер означает "здесь столько параметров, что их в одном месте тупо не проставить, и нужно отсюда взять один, отсюда другой, соединить хитро" и тд. поэтому он билдер.

3. Есть там место где тебе очень нравится if ($a instanceof Class) и раз 20. Можешь закинуть имена классов в качестве ключей в массив и тупо чекать isset(). Разве что ты планируешь наследоваться, тогда isset() будет не очень. Но в любом случае форич упростит этот код строк на 30

4. по эксепшенам как бы не превратилось в холивар, но смело бросай стандартные Пхпшные эксепшены с небольшой ремарочкой - оберни их в свой неймспейс (в этом случае категории останутся стандартные, а твой модуль будет бросать как бы "свои собственные"). Для случаев когда тебе очень нужно что-то логировать и вытягивать из эксепшена, я бы предусмотрел как и у тебя ClientException только вместо Response просто пхал бы туда ...$arguments. Что кинешь - то и достанешь потом. То есть стандартные мессаг, код и предыдущий будет как у всех эксепшенов - а аргументы - чтобы их логировать. Не думаю что очень ты выиграл от того, что сделал эксепшен в котором первым параметром обязательно респонс. Теперь он может быть использован только здесь и нигде больше. Если эксепшен приводит к каким-то действиям кроме "поймать", "перебросить", "уведомить" или "залогировать" - очень вероятно что это не эксепшен. Хотя такой вариант может быть почему нет.

5. Интерфейсы лежат вместе с классами это не плохо, но рядом с интерфейсом нет такого же класса, а идут другие, реализующие интерфейс. Похоже на абстракцию, которая могла бы быть. ActionInterface.php, Action.php и папка Action где уже лежат Actions. Но это тоже косметика. В самих экшенах я бы все таки добавил слово Action несмотря на то что неймспейс тоже Action. Если открыть много файлов в редакторе, то сверху одинаковые имена могут дублиться, а так видно - что тут речь про экшен, а там не про экшен.

6. Вот что мне показалось странным - у тебя куча "экшенов" подразумевающих что это "действие". Но там внутри лежат сущности названные "Действие". Ну то есть ты создал много классов задача которых входные аргументы проверить по типу (кинуть InvalidArgumentException если косяк, по сути), и больше ничего. Ну у них есть геттеры и свойства. Это состояние никак не меняется, оно просто устанавливается и валидируется. Можно. Выглядит красиво, аккуратно. Но бегает у меня сзади призрак который намекает что зарраза, столько файлов создал, он не задолбался? Впрочем если апишка бота непредсказуема - типа на одно возвращает одно, на второе другое и нет ничего общего - то такой способ валидации приемлем. К сожалению кроме разработчика который будет на базе этой штуки лепить, никто не сможет на них никак среагировать, т.к. InvalidArgumentException это в первую очередь "уведомить разработчика что сюда попало то, что не должно", отловить и перевести на несколько языков такую ошибку не получится. Собственно поэтому прибегают к валидации там, где этой штукой будет пользоваться менее опытный разработчик или пользователь.

Наверное всё, можно пообщаться в той же телеге, если интересно детальнее. В целом понравилось
Ответ написан
@grinat
Даже код смотреть неохота, коммиты вида refactoring part 2 уже говорят обо всем. Нафига думать, зачем описывать имзенения, если можно просто делать: refactoring part 2, refactoring part 3, code part 1, code part 2, my mom love pizza
Ответ написан
Комментировать
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы
19 апр. 2024, в 05:01
999999 руб./за проект
19 апр. 2024, в 03:52
1000 руб./за проект
19 апр. 2024, в 03:01
1000 руб./за проект