Pashosi - TennisMatchScoreboard.md

https://github.com/Pashosi/TennisMatchScoreboard
Ревью от Виктора @csatom

Немного посмотрел код, напишу что можно улучшить

• наличие JavaScript в шаблонах

• регулярные выражения в шаблонах

• к функциям есть строка документации

• в статике не требуется добавлять файлы init.py, не надо в view, static, templates

• файл manage.py обычно называют main.py или app.py, но лучше main

• в файле test_tennis_match.py импортируются, но не используются модули

from src.controller import Controller
from src.model import Model

• в файле tennis_match.py импортируется, но не используется модуль

from jinja2 import Template

• в файле router.py в классе Router есть переменная на 33-й строчке

n = 1

она нигде в логике функции не используется, но зачем-то инициализируется что-то имелось в виду с целым числом n, возможно ранее она как-то использовалась, а потом был произведен рефакторинг, и эта переменная стала не нужна

• в файле match_score_post_handler.py в функции get_data_match тоже есть не используемая переменная n = 1

• в файле controller.py в функции get_data_match тоже есть не используемая переменная n = 1

• функция def get_matches() в классе Model. В нескольких местах этой функции используется неправильное сравнение с None. Для сравнения надо использовать конструкцию is None или is not None. То есть where(MatchesModel.winner_fk.isnot(None)), а не where(MatchesModel.winner_fk != None) SQL и Python по-разному обрабатывают None. В SQL используется "IS NOT NULL", а питон нашу конструкцию "winner_fk != None" преобразует к виду "winner_fk is not None".

• в проекте присутствуют пустые файли миграции "9c516209d9ae_fix_models.py" и "c290334fcfbf_fix_models2.py" они никак не меняют состояние БД, upgrade и downgrade пустые

• в файле env.py гвоздями прибита строка подключения к БД f'mysql+mysqlconnector://root:root@localhost:3306/{db_config["mysql"]["name"]}' в учебном проекте так сделать можно, но гитхаб даже может прислать уведомление об проблеме безопасности по этой строчке можно будет подключиться к твоей БД, выполнить там любой запрос (удалить все данные например) лучше сделать переменные типа DB_USERNAME и DB_PASSWORD, которые сложить в файлик .env и из него тянуть эти переменные окружения тогда строка будет примерно такого вида: f"mysql+mysqlconnector://{self.DB_USERNAME}:{self.DB_PASSWORD}@{self.DB_HOST}:{self.DB_PORT}/{self.DB_NAME}"

• переменные окружения используются, но имена не очевидны

NAME=ваш_username_mysql
PASSWORD=ваш_password_mysql

так как в проекте может использоваться несколько имен и паролей. лучше называть их более явно: DB_USERNAME и DB_PASSWORD

• в файле controller.py функция names_check лучше переименовать ее в check_names 27 строчка закомментирована

# model.add_player(name)

может быть сработало бы такой вариант:

new_player = model.add_player(name)
players.append(new_player)

логика в этом иф непонятна if len(players) < 2: при каких условиях тут может быть меньше 2 ? если в функцию приедет словарь не равный по длине 2 ошибка raise Exception('игрок не может играть сам с собой') как будто бы обрабатывается на стороне фронта и тут она никогда не сработает

• в файле controller.py функция new_match(self, environ=None, test_data: dict = None): аннотации типов не до конца указаны - test_data - это дикт, а по умолчанию присваивается к None добавить полностью аннотации, например

new_match(self, environ: str | None = None, test_data: dict | None = None):

• где-то по коду есть закомментированные куски кода. они должны быть или раскомментированы или удалены для заметок хорошо подходит ставка строковых примечаний TODO

• логика игры составлена одним классом TennisMatch. Такой подход не позволит создать новый матч с другим способом подсчета очков Матч может состоять из 3-х или 5-ти сетов. Какие-то турниры играются без тай-брейка. А некоторые турниры добавили супер тай-брейк. В таком случае придется писать для каждого турнира свой класс, и всю логику в нем. Правильно будет сделать три отдельные сущности: TennisMatch, TennisSet, TennisGame Описать взаимодействие между ними. При такой подходе добавить новые правила подсчета очков будет легко. И покрытие тестами будет для каждого класса отдельно

• в файле router.py строка 64:

match = TennisMatch(data)

TennisMatch ожидает на входе тип данных MatchesModel, а получает словарь поэтому дальше получается тоже не верных тип данных в поле score тип JSON, который не поддается индексации, а в инициализаторе к нему происходит обращение как к словарю

data_match.score['player1']['games']