Code review #1
Description
Фидбек по тестовому заданию.
Где-то я уже видел это тестовое...у кого-то в репе или присылали в чатик :) если интересно покричи в чатик, может кто-то уже сталкивался с этой конторой)
Хорошая работа, учитывая, что нет четких требований и ты только в начале пути. Ничего преступного не заметил, есть только некоторые замечания, которые неплохо бы исправить в следующем проекте.
Когда мы делаем тестовое, наша задача показать как мы умеем:
- проектировать приложение
- учитывать возникающие в дальнейшем проблемы
- предусматривать масштабируемость, расширяемость, отказоустойчивость и вот это вот все
- пользоваться инструментами (гит, докер, ЯП, нджинксы и прочее)
Если в ТЗ нет четкого требования к стеку, ревьюер смотрит на выбранные тобой инструменты (фреймворки, БД и т.д.) и оценивает насколько ты принимаешь осознанные решения. Поэтому если мы берем джангу - мы должны ответить на вопрос почему именно её, а не какой-нибудь ассинхронный FastAPI, если мы берем Postgres - нужно понимать почему его (а не MariaDB, например) и точно ли нам нужна реляционная база или неплохо бы использовать MongoDB, если предполагается частое чтение (в данном случае РСУБД наш бро, но нужно рассматривать все варианты и оценивать их применимость).
Например, я беру джангу только ради админки или если мы прям на старте планируем писать тяжелейший монолит. Для всего остального есть FastAPI / Flask в зависимости от требования к производительности.
В работе хотелось бы видеть больше решений, позволяющих:
- запустить проект в одну команду (make, docker, django management command)
- масштабировать приложение (сейчас у тебя за это отвечает гуникорн, который будет стоять в очередь к SQLite).
Именно это будет выделять тебя среди оравы джунов.
К сожалению, я не работаю с DRF, поэтому мне сложно дать объективную оценку по твоим вьюшкам. Оставлю только вот эту статью (цикл статей, на самом деле), которую хочу когда-нибудь сам изучить, но меня как-то от DRF господь уводит каждый раз :)
Было бы круто, если бы кто-то из ребят смог вьюшки оценить.
Так же и с запросами, Django ORM знаю плохо (работаю с алхимией в основном), выглядит нормально, но лучше знающим заглянуть.
Что было хорошо:
- Хорошая админка с ништяками (но лучше избегать кириллицу)
- Сериалайзеры практикумовские - это хорошо :)
- Есть достаточно подробная инструкция по запуску проекта
- работа в целом чистая и аккуратная
- выполнены все требования "концепции"
Что можно было бы улучшить:
- Когда мы в ридмихе держим ссыль на поднятый проект, то самое неприятное - это когда читающий тыкает и получает
Not Found. Все-таки лучше или хостить на своем VPS или не держать ссыль (если такого требования не было в ТЗ) перенести БД с SQLite. Тут такое дело...SQLite в тусовке считается плохим тоном, если мы не говорим о мобильных приложениях или IoT. Поэтому лучше всего Postgres поднимать в контейнере и сразу писать код для psql - это очень просто и почти не занимает времени, нужно просто приучить себя :) Более того, ты говоришь о переходе в разделе "Перспективные доработки проекта". Мигрировать с SQLite на PSQL, в проде очень больно (посмотри как работают агрегирующие функции, как они работают с датами). Так же выбор БД - это центральная задача при планировании архитектуры приложения, представь, что у нас будет 100 тысяч пользователей, как себя поведет SQLite? И вообще, сколько пользователей мы сможем держать на этой базе? Сможем ли мы запустить проект в прод на этой БД?контейнеризировать... Опять же, докер для питонистов и гошников - это мать родная. Сейчас всё вокруг клауд-френдли, разработка предполагает поднятие на своей машине небольшого кластера контейнеров, поэтому рассматривать контейнеризацию, как какую-то доработку я бы не стал, а сразу заворачивал в докер.Django==2.2вышел в апреле 2019 года и ему еще 2 месяца осталось, после чего поддержка этой версии будет прекращена. Не самый лучший выбор :)- Когда мы проектируем API важно также предусмотреть расширяемость и масштабируемость. Например, очень не хватает версионирования API.
- к чему этот requirements или этот?
dating-site/api/api/проект лучше называтьconfig, ведь в нем лежат сеттинги и настройки наших серверов.favicon\.ico$лучше в регулярке делатьico|png, в 70% случаев фавикон приносят именно в png и лучше сразу это прописать на бекэнде :)- считается хорошим делом разносить сеттинги сразу на несколько сред. Вот пример. Это позволит спрятать из продкода всякие
localhost,django debug toolbarи прочие штуки, которые нас не сильно интересуют (так же можно разнести и зависимости, например) - автонагенеренные комментарии лучше убирать из репозитория. Они бесполезны, но делают сложнее чтение кода.
- порт лучше тоже спрятать
- мы живем в век толирастии, поэтому тут лучше предусмотреть "другой" (если мы говорим о сайтах знакомств, то там всякие кейсы могут быть). Ну и я бы хранил в БД Enum, вместо букв, либо наоборот male/female/other (подумай о тех, кто будет запросы в БД писать)
- все поля у тебя обязательные к заполнению, в некоторых местах ты пишешь
blank=False, а в некоторых ничего не пишешь, если фолс по дефолту у модели, то для чего явно указывать в других полях? - поле с паролем лучше указать CharField, как и поля имени и фамилии.
- почему не используешь f-string при формировании строк?
- было бы круто иметь какой-нибудь скрипт, который наполнил бы БД тестовыми данными (гугли
django loaddata,django manage command,factory boy).
Критичные замечания:
POST-запрос: /clients/create/этот URL не по RESTfull. для создания нового пользователя не нужно использоватьcreate. Очень крутой гайд по ресту от мелкомягких - https://github.com/microsoft/api-guidelines/blob/vNext/Guidelines.mdСоздание отметки у участника: GET-запрос. Всегда любое создание объекты мы делаем с помощью POST-запроса.- Вся статика в репе, так быть не должно, статика собирается в момент деплоя.
- Нет миграций в репозитории. Понятно, что проблем в данной ситуации не будет, так как репа используется в качестве витрины, но в рабочей репе файлы миграций обязаны быть, поэтому нормально выкладывать "первую" миграцию в репу.
except BaseExceptionпо рукам себя бей за неконкретные экзепшены. Код схавает except и дальше пойдет, как ни в чем не бывало, хотя отправка писем у тебя ключевая функция и лучше бы в этой ситуации упасть всему приложению.