https://github.com/Awakary/TennisScoreBoard
Ревью от Сергея @grandpraline
Disclaimer ревью не претендует на исчерпывающее нахождение всех ошибок и финальную истину, но старается принести пользу
- проект в целом работает, основной функционал поддерживается, счет на первый взгляд ведется корректно
- есть пагинация страниц завершенных матчей
- есть тесты на unittest
- есть кастомные эксепшны
- есть деплой на удаленный сервер (видел в начале, я так понял ты его погасила) и докер-композ (по нему есть замечания и предложения ниже)
- применены PostgreSQL, SqlAlchemy, unittest, Jinja2, wsgiref (референсная реализация WSGI из базовой библиотеки Питона)
(не в порядке значимости)
- DAO - думаю, что хорошей идеей было бы разделить DAO по моделям и не смешивать игроков с матчами. Так было бы ближе к SRP и при росте приложения было бы легче вносить изменения. Также передавать DAO потребителям (хэндлерам, сервисам) через конструктор. Тогда можно было бы DAO создать на уровне роутера, передать в конструктор и забыть про него.
- Показалось, что в Пагинации многовато хардкода. Я не стал сильно погружаться, с чем это связано, и действительно ли это нужно. Если нужны настройки с числовыми значениями, то предложил бы их передавать через настройки (например, брать из env и потом как-то использовать). Также можно было бы использовать какой-то Enum c набором констант и их значений, чтобы увеличить читаемость, что тут происходит :). Саму Пагинацию тоже бы передавал потребителям через конструктор.
- Наблюдение про использование зависимостей распространяется и на другие классы, почитай про Dependency Injection.
- Ты не используешь тайпхинты. Рекомендую начинать это делать, обратить внимание на это. В коде продакшн-уровня это будет нужно делать, начинай привыкать постепенно, начни с простых тайпхинтов к аргументам и возвратам функций.
- Рендерер достаточно сильно завязан на конкретные литеральные значения названий шаблонов, а также поля словаря. Что-то может пойти не так. Улучшить ситуацию можно было бы, как мне кажется - использовать Енам для названий шаблонов, а для передачи значений полей модели использовать какой-то датакласс (dataclass) или pydantic-модель (кстати, обрати внимание на pydantic в следующем проекте)
- Мне показалось, или Response всегда идет со статусом 200? И в случае ошибки тоже. Помнишь, в третьем проекте у нас были разные статусы ответов?
- Service это не просто Сервис :) важно давать понятные имена тем кусочкам кода, которые мы делаем. Чтобы потом было легче в них ориентироваться
и не тратить мыслетопливо :) Над сервисом я бы прямо предложил поработать и в плане нейминга методов, и разделения их, и DRY (change_AD_to_forty_1, change_AD_to_forty_2?:)).
p = self.player_score['points'] g = self.player_score['games']Ранен) Не рекомендую так называть переменные. Только если хочешь стать оунером этого сервиса и никого не подпускать) - Хорошо бы еще разделить счет на понятные уровни, вроде гейма, сета, матча, можно сделать через классы.
- Docker-compose
- версия докер-композа (ты указала версию питона, но тут в поле version пишется версия Докер-композа, сейчас уже вообще не пишется)
- порт-байндинг (ты указала одновременно и параметр network mode, и port bindings, они конфликтуют, нужно что-то одно)
- volume: для сохранения данных при остановке контейнера хорошо бы добавить volume
Успешно сделан проект, есть над чем поработать из уже пройденного - тайпхинты, нейминг, разделение ответственности. Рекомендую начинать использовать pydantic для валидации. Передавать данные с использованием удобных объектов, словари это не самая удобная вещь. Почитать про dependency injection. В плане тестирования некоторый опыт получен, предлагаю переходить на pytest и осваиваться в нем, тк я вижу, что он достаточно часто используется, есть своя специфика. Предлагаю поработать над Сервисом в качестве самостоятельного домашнего задания и довести его до такого состояния, чтобы он нравился тебе лично. На таком упражнении можно было бы потренировать тайпхинты, передачу данных с помощью объектов, нейминг.