complete http part of gophermart #1
Conversation
|
В этой версии я закончил все запросы из части про Gophermart, и пока не делал расчёт награды за каждый заказ, как раз буду приступать после этой части, хотелось бы чтобы отсмотрели архитектурные моменты, чтобы пока далеко не ушёл - исправить их |
cmd/gophermart/main.go
Outdated
| authHandler := handlers.NewAuthHandler(&handlers.AuthHandlerOptions{ | ||
| UserService: userService, | ||
| }) | ||
| balanceHandler := handlers.NewBalanceHandler(&handlers.BalanceHandlerOptions{ |
There was a problem hiding this comment.
а зачем тут везде Options? почему не прокинуть просто как зависимости?
|
|
||
| func (w *CalculateOrderAccrualWorker) Start(ctx context.Context) { | ||
| log.Println("Start calculate_order_accrual worker") | ||
| ticker := time.NewTicker(time.Second * 5) |
| continue | ||
| } | ||
|
|
||
| for _, order := range orders { |
| log.Println("[checking_order_accrual] putting worker to rest for", seconds, "seconds") | ||
|
|
||
| w.isResting = true | ||
| timer := time.NewTimer(time.Second * time.Duration(seconds)) |
There was a problem hiding this comment.
а зачем тут это? и тут не надо локи? если у тебя это в горутинах
|
|
||
| err = w.balanceActionRepository.Save(ctx, order.UserID, order.OrderID, *orderInfo.Accrual) | ||
|
|
||
| if err != nil { |
There was a problem hiding this comment.
не критикал, но получается может выполниться запрос SetOrderCalculatingResult? но потом balanceActionRepository.Save моожет не сохранить, тут целостность данных не ломается? как это потом будет обработано?
| return | ||
| } | ||
|
|
||
| err = w.balanceActionRepository.Save(ctx, order.UserID, order.OrderID, *orderInfo.Accrual) |
There was a problem hiding this comment.
а если *orderInfo.Accrual будет nil?
| } | ||
|
|
||
| func (w *OrderAccrualCheckingWorker) processOrder(ctx context.Context, order *domain.UserOrder) { | ||
| orderInfo, err := w.getInfoFromAccrualSystem(order.OrderID) |
cmd/gophermart/main.go
Outdated
| r.Post("/register", authHandler.Register) | ||
| r.Post("/login", authHandler.Login) | ||
|
|
||
| r.Get("/orders", middlewares.AuthMiddleware(http.HandlerFunc(ordersHandler.GetOrders))) |
There was a problem hiding this comment.
можно сделать r.Group и внутри использовать сразу один раз middleware
|
|
||
| for _, order := range orders { | ||
| go func(o domain.UserOrder) { | ||
| go w.processOrder(ctx, &o) |
There was a problem hiding this comment.
вот тут зачем горутина в горутине, я не там написал
…d transaction support
internal/facade/order_accrual.go
Outdated
| SetOrderCalculatingResult(ctx context.Context, orderID string, status string, accrual float64) error | ||
| } | ||
|
|
||
| type OrderAccrualFacade struct { |
cmd/gophermart/main.go
Outdated
| userOrderRepository, | ||
| orderAccrualFacade, | ||
| appConfig.AccrualSystemAddress, | ||
| workersWg, |
There was a problem hiding this comment.
хорошей практикой когда вся работа с waitGroup в одном файле, поэтому рекомендую так не делать
сейчас работа с wg размазана по всему файлу, не прозрачно получается
cmd/gophermart/main.go
Outdated
| log.Fatal(err) | ||
| } | ||
|
|
||
| dbPool.Close() |
There was a problem hiding this comment.
лучше поднять к месту где инициализируется и сделать в defer, чтобы было рядом
| ticker := time.NewTicker(time.Second * 5) | ||
|
|
||
| go func() { | ||
| defer ticker.Stop() |
| if err != nil { | ||
| var retryAfterError domain.RetryAfterError | ||
| if errors.As(err, &retryAfterError) { | ||
| ticker.Reset(time.Second * time.Duration(retryAfterError.Seconds)) |
There was a problem hiding this comment.
лучше тикер двигать не в каждой горутине, а использовать пакет errgroup например или самому сделать аналог, получил ошибку - остановил или дал отработать остальным горутинам и уже после подвинул тикер
|
|
||
| log.Println("Start checking_order_accrual worker") | ||
| ticker := time.NewTicker(baseTickerDuration) | ||
| w.wg.Add(1) |
| } | ||
| } | ||
|
|
||
| func (w *OrderAccrualCheckingWorker) Start(ctx context.Context) { |
There was a problem hiding this comment.
Написал тест на эту функцию и запустил с флагом -race
спойлерить не буду, попробуй сам найти
| ) | ||
|
|
||
| type BalanceActionRepoMock struct { | ||
| Storage []domain.BalanceAction |
There was a problem hiding this comment.
Такой подход не очень практичный
- Все пакеты становятся завязанные на пакет mocks
- Мок содержит в себе много функций
- Попробуй запустить тесты параллельно, у тебя тут хранится стейт, к чему это приведет?
очень рекомендую убрать моки в каждый пакет и моки сделать через mockery или аналог, чтобы состояние мока можно было менять и задавать в кейсах
…mocks, add gzip compression, wrote some tests
internal/services/withdrawals.go
Outdated
| func (s *WithdrawalsService) WithdrawBalance(ctx context.Context, userID int, orderID string, amount float64) error { | ||
| userBalance := s.balanceActionRepository.GetCurrentBalance(ctx, userID) | ||
|
|
||
| if userBalance-amount < 0 { |
There was a problem hiding this comment.
ты получил userBalance раньше, а тут проверяешь, но ведь в этот момент кто-то уже мог изменить баланс, и теперь он в базе будет с новым значением а эта проверка пройдет, я правильно понимаю?
No description provided.