BarAPI#2
Conversation
AshasTob
left a comment
AshasTob
left a comment
There was a problem hiding this comment.
Very good PR overall. Solid changes.
A couple of "just in my taste" kind of comments here
A couple of "typo here" kind of comments
A couple of "should be async" comments :)
Also a bit of Exception vs Result thing going on in a repository.
| return BadRequest(ModelState); | ||
| } | ||
| int affacted = await _orderRepository.Upsert(item); | ||
| if(affacted == -201) |
| } | ||
| else | ||
| { | ||
| return null; |
There was a problem hiding this comment.
I don't like signature of this repository. Here's fun article (its old)
https://hinchman-amanda.medium.com/null-pointer-references-the-billion-dollar-mistake-1e616534d485
Basically returning null here makes app layer more difficult.
Personally, I , in this case would be using a Failed state of Result object (from suggested comment above)
Alternatively an exception.
|
|
||
| public async Task<int> Upsert(Order order) | ||
| { | ||
| bool ord = _barDataBase.Orders.Contains(order); |
There was a problem hiding this comment.
I am not EF proficient user here.
But if this method goes to database it should be async. Why not use "Get" method from above? If it does not go to database - then i dont understand how it works.
I think something wrong is going on here
There was a problem hiding this comment.
I make method async because I use async method like FindAsync() , SaveChangesAsync(). But there are appropriate non-async method.
There was a problem hiding this comment.
have you tested this method? Does it work as intended?
I still think that all operations with external dependencies(such as SQL DB) should be done asynchronously
| { | ||
| public class OrderTimeOut | ||
| { | ||
| private readonly BarDataBase _barDataBase; |
There was a problem hiding this comment.
This works.
But if we are using repository approach, i think an Order repository should be referenced here. It is very important to be super consistent in your application to avoid unexpected regressions later on
This reverts commit 9ce03a9.
| public async Task<Order> Get(int id) | ||
| public Task<Order> Get(int id) | ||
| { | ||
| Order order = await _barDataBase.Orders.FindAsync(id); |
There was a problem hiding this comment.
а чому ти вирішив перейти від асинхронного FindAsync до синхронного Find?
мені здається що так як було - функціонально працювало так само, але було асинхронно тобто більш оптимізовано
| _barDataBase.Orders.Update(ord); | ||
| order.Status = OrderStatus.Finalized; | ||
| finalizedOrdersSum += order.TotalPrice; | ||
| await _orderRepository.Upsert(order); |
There was a problem hiding this comment.
Є одна проблема тут.
У тебе виходить що на приклад якщо є 10 замовлень і ти закривав вже 5 з них в циклі, а потім 6тий запит до бази падає по якійсь причині. На приклад мережа, або навантаження. виходить що ти зробив тільки половину роботи і стан який ти зберіг в базу - є не фінальним.
часто зберігати "частково" правильний результат - гірше ніж не зберігати нічого взагалі.
Я вважаю, що це не критично в даному випадку але почитай про патерн UnitOfWork
https://www.c-sharpcorner.com/UploadFile/b1df45/unit-of-work-in-repository-pattern/
|
|
||
| public BlobStorage() | ||
| { | ||
| _blobServiceClient = new BlobServiceClient(GetEnvironmentVariable("BlobStorageConnectionString", EnvironmentVariableTarget.Process)); |
There was a problem hiding this comment.
а як ти ці конфіги передаєш у функцію? у Ажур порталі?
Це буде працювати :)
але це не ідеальний спосіб. Тепер твій клас BlobStorage має пряму залежність на змінні середовища.
Як би я це робив:
- Зробив би окремий клас ReportRepositorySettings чи щось таке
- На стартапі в цей клас би записував дві проперті BlobStorageConnectionString, OrderTimeOutReportContainer
- BlobStorage би залежав на ReportRepositorySettings і юзав вже ці проперті
Із таким апроачем ми можемо заповнювати оці 2 проперті без залежності того звідки вони йдуть
2 participants
No description provided.