https://github.com/PavelFurochkin/TennisMatchProject
Ревью от Сергея @grandpraline
Disclaimer ревью не претендует на исчерпывающее нахождение всех ошибок и финальную истину, но старается принести пользу
- проект в целом работает, основной функционал поддерживается, счет ведется корректно (мне не удалось проверить деплой, с моих устройств ответа получить не смог. локально все запустилось)
- есть пагинация страниц завершенных матчей
- есть примеры тестов на unittest
- применены SqlAlchemy, unittest, Jinja2, waitress, whitenoise
(не в порядке значимости)
- Не используются тайпхинты. На этом этапе уже пора начинать их использовать, потребность будет только расти. Обрати внимание на эту тему.
- Пока не используются миграции. В 5м проекте можно будет с ними познакомиться, также нужно будет познакомиться с Alembic в какой-то момент.
- Захардкожена БД и адрес к ней. Предлагаю попробовать загружать с использованием PydanticSettings из .env файла, pydantic_settings уже есть в requirements проекта. В совокупности можно попробовать добавить миграции (для первоначального наполнения базы данных при старте) и заменить базу данных на PostgreSQL, чтобы попрактиковаться.
- Предлагаю добавить README в проект, чтобы лучше представить этот проект на гитхабе + приложить инструкцию по запуску проекта.
- Думаю, что хорошо бы вынести jinja.py и game_score_converter.py из папки с шаблонами в отдельный пакет, jinja.py переименовать в renderer.py, так было бы чуть организованней.
- Хотелось бы развить тему с кастомными эксепшными. Они есть, но выделен только один тип MatchNotFoundByUUID, а остальные OtherError, это немного не прогерфрендли :) Также код этих эксепшнов не отличается. Можно предложить назвать ошибки понятными именами, а в их код включить сразу какое-то текстовое сообщение, соответствующее этой ошибке. Может быть дополнять какими-то аргументами, если это уместно (например, если ошибка произошла в каком-то конкретном аргументе, то включать его в сообщение).
- В контроллерах предложение передавать зависимости в конструктор, чтобы уменьшить связанность. Сейчас ДБСервис иницииализируется внутри конструктора BaseController, его можно вынести наружу. Также предлагаю вынести зависимость Renderer, рассмотреть вариант создать и вынести в виде зависимости Parser, который бы парсил запросы. Думаю, что можно добавить сервис пагинации, и тоже его передавать.
- DBService очень много всего делает и превратился в комок. Он создает таблицы, обновляет БД (update_db на самом деле обновляет счет - проблема с неймингом!), получает игроков, проверяет их доступность, добавляет матчи и занимается пагинацией. Предложение разделить этот сервис на части, специализировать их. +Обрати внимание на паттерн DAO - Data Access Object, который мог бы помочь разделить ответственность за работу с базой данных в отношении конкретных объектов - Player, Match и разнести бизнес-логику (которая могла бы остаться в Сервисе) и работу с базой данных (которая была бы абстрагирована в DAO). Также, валидация находится в методах, которые работают с БД, думаю, что валидацию можно было бы делать раньше, например в контроллере, и сразу возвращать ошибку, если что-то не так. Мы как будто многовато возвращаем из методов сервиса, получается, что мы используем модель базы данных, работаем с ней в приложении. Почитай про DTO, думаю, что модель БД не должна путешествовать по приложению, т.к. это повышает общую зависимость от нее.
- Настройки пагинации можно было передавать через настройки (BaseSettings), чтобы не хардкодить, или вынести на уровень класса Пагинации.
- Для очков в гейме и других констант рекомендую использовать Enum
Поздравляю с завершением проекта! Рекомендую обратить внимание на: тайпхинты (считаю очень важным использовать), нейминг функций и модулей, разделение ответственности. В плане тестирования некоторый опыт получен, предлагаю в следующем проекте использовать pytest. Предлагаю поработать с замечаниями в качестве самостоятельной практики, почитать про концепцию DAO и примеры использования, также почитать про инъекцию зависимостей. Может быть затащить PostgreSQL в проект, чтобы не задерживаться на SQLite и потренировать подключение к БД с использованием settings.