Review PR #9
Review PR #9
Conversation
4 | фикс ci
5 order post
| @@ -0,0 +1,23 @@ | |||
| help: ## Display this help screen | |||
| @@ -0,0 +1,44 @@ | |||
| version: '3.5' | |||
There was a problem hiding this comment.
За докеркомпоуз тоже лайк!
| return service.AccrualOrder{}, err | ||
| } | ||
|
|
||
| if resp.StatusCode() == http.StatusNoContent { |
There was a problem hiding this comment.
Я бы здесь все же возвращал кастомный ErrNotFound обернутый в поясняющий текст:
return service.AccrualOrder{}, fmt.Errorf("order not found in accrual: %w", ErrNotFound) 
There was a problem hiding this comment.
✔️ Не уверен что сделал так как предполагалось - я возвращаю ошибку, но и объект тоже по-прежнему возвращаю.
И в вызывающей функции логирую этот момент с warning.
internal/container/router.go
Outdated
| c.Abort() | ||
| return | ||
| } | ||
| c.Set("current_user_id", userID) |
There was a problem hiding this comment.
Использовать встроеные типы в качестве ключа контекста не рекомендуется тк может быть коллизия. Плюс не очень классно что хэндлеры должны знать ключ и тип значения для того чтобы достать userID.
Лучше всего сделать работу с контекстом как описано здесь:
https://bongnv.com/blog/2021-04-10-safely-use-go-context-to-pass-data/
There was a problem hiding this comment.
✔️
Спасибо за замечание. Я сделал отдельные методы, которые немного облегчают задачу и позволяют хендлерам не знать ключ.
К сожалению, как я понял, заиспользовать специальный тип для ключа не получается с gin.Context. Ну или это не так просто.
gin-gonic/gin#1123 (comment)
pkg/logger/zerolog_adapter.go
Outdated
| func (z *ZeroLogAdapter) Warn(err error) { | ||
| z.Logger.Warn().Stack().Err(err).Msg("") | ||
| } | ||
|
|
||
| func (z *ZeroLogAdapter) Err(err error) { | ||
| z.Logger.Error().Stack().Err(err).Msg("") | ||
| } | ||
|
|
||
| func (z *ZeroLogAdapter) Fatal(err error) { | ||
| z.Logger.Fatal().Stack().Err(err).Msg("") | ||
| } |
There was a problem hiding this comment.
Для ошибок, варнингов и фаталов нам тоже нужна возможность добавлять сообщения пояснительные, иначе понять где этот лог записан будет нвеозможно
There was a problem hiding this comment.
Я так понял ты эту проблему решаешь выводом стэктрейса но это плохая практика - стектрейс дорогой и его неудобно читать по сравнению с сообщением "unable to get user blah-blah"
There was a problem hiding this comment.
✔️ вместо трейса теперь есть второй параметр msg.
| return | ||
| } | ||
|
|
||
| u.logger.Err(err) |
There was a problem hiding this comment.
Давай напишем пояснение к ошибке (и сделаем метод который это позволяет делать), а-ля:
u.Logger.Errf(err, "unable to create new user")
| } | ||
|
|
||
| user, err := u.userService.CreateNewUser(credentials) | ||
| if err == nil { |
There was a problem hiding this comment.
Давай уменьшим вложенность, сделав happy-path в конце функции:
user, err := u.userService.CreateNewUser(credentials)
if errors.Is(err, service.ErrPwdIsNotValid) || errors.Is(err, service.ErrLoginIsNotValid) {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}
if err != nil {
u.Logger.Errf(err, "unable to create new user")
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
return
}
tkn, err := token.Generate(user.ID, u.apiSecret, u.tokenLifespan)
if err != nil {
u.logger.Errf(err, "unable to generate new token")
c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
return
}
c.Header("Authorization", fmt.Sprintf("Bearer %s", tkn))
c.JSON(http.StatusOK, gin.H{"message": "success"})There was a problem hiding this comment.
✔️ тут и в других хендлерах порефакторил этот момент.
| } | ||
| order, err := u.orderService.AddOrder(currentUserID, orderNumber) | ||
| if err != nil { | ||
| if err == service.ErrEntityAlreadyExists { |
There was a problem hiding this comment.
Лучше использовать errors.Is
| userID := getCurrentUserIDFromContext(c) | ||
|
|
||
| balance, err := u.userService.GetBalance(userID) | ||
| if err != nil { |
There was a problem hiding this comment.
тут надо бы залоггировать
| func getCurrentUserIDFromContext(c *gin.Context) int { | ||
| currentUserIDAsAny, ok := c.Get("current_user_id") | ||
| if !ok { | ||
| panic("cannot get current user id. check the endpoint is protected.") |
There was a problem hiding this comment.
Мы никогда не паникуем в хэндлерах, если ты хочешь зафиксировать наличие мидлвари перед хэндлером то лучше написать на это тесты, а здесь просто возвращать ошибку
There was a problem hiding this comment.
✔️ переделал на возврат ошибки.
| } | ||
| defer rows.Close() | ||
|
|
||
| return hydrateOrders(rows) |
There was a problem hiding this comment.
Кажется что это не особо уменьшает дублированние кода (эта функция используется в двух местах только) зато сильно ухудшает читаемость. Давай тело функций гидраторов вставим непосредственно внутрь метода
internal/service/order_service.go
Outdated
| case entity.AccrualStatusRegistered: | ||
| fallthrough | ||
| case entity.AccrualStatusProcessing: | ||
| order.Status = entity.OrderStatusProcessing |
There was a problem hiding this comment.
Можно написать два значения в кейсе через запятую чтобы не делать fallthrough:
case entity.AccrualStatusRegistered, entity.AccrualStatusProcessing:
order.Status = entity.OrderStatusProcessing
internal/service/order_service.go
Outdated
| for _, order := range orders { | ||
| accrualOrder, err := o.AccrualClient.GetOrder(order.OrderNumber) | ||
| if err != nil { | ||
| o.Logger.Err(err) |
There was a problem hiding this comment.
Тут и далее хорошо бы пояснения к логу
pkg/utils/utils.go
Outdated
| func InvokeFunctionWithInterval(duration time.Duration, functionToInvoke func()) { | ||
| ticker := time.NewTicker(duration) | ||
| for { | ||
| <-ticker.C | ||
| functionToInvoke() | ||
| } | ||
| } |
There was a problem hiding this comment.
было бы неплохо сюда еще прокинуть контекст и внутри цикла делать select по <-ticker.C и <-ctx.Done - в ctx.Done кейсе делать return
| if err != nil { | ||
| if err == service.ErrEntityAlreadyExists { |
There was a problem hiding this comment.
Опять же, лучше уменьшать вложенность когда это возможно (а здесь это возможно)
internal/container/container.go
Outdated
| "order_number" VARCHAR NOT NULL, | ||
| "status" INTEGER NOT NULL, | ||
| "accrual_status" INTEGER NOT NULL, | ||
| "accrual" DOUBLE PRECISION NOT NULL, |
There was a problem hiding this comment.
Ну и самое главное - арифметика с сырыми floating-point значениями крайне нерекомендована при работе с деньгами. Поэтому я просил как можно скорее принести работу на проверку ментором.
Флоты в силу своей реализации в компутере имеют проблемы с точностью представления чисел и округление тут не спасает: например нам надо поделить 100 рублей между тремя юзерами. Мы делаем
var sum float64 = 100
var usersCount float64 = 3
res := sum/usersCount // округляется в 33, куда делся рубль?Есть два решения этой проблемы
- Хранить в БД деньги интами в копейках. В коде складывать, вычитать, умножать и делить (не забывая при этом проверять остаток, чего нельзя делать с флотами) тоже используя инты. Правда когда будем общаться с клиентом надо будет делить/умножать на 100, тк ТЗ требует два знака после запятой
- Работать в коде с этим пакетом https://pkg.go.dev/github.com/shopspring/decimal, хранить в базе в numeric
Я рекомендую первый подход
There was a problem hiding this comment.
✔️ переделал на вариант с копейками.
…вместе с заказом со статусом Unregistered ошибку, которая будет залогирована
…я и умножения бонусов.
11 review
No description provided.