Bug fix Rating #311
Bug fix Rating #311
Conversation
SergeyNovitsky
requested review from
h4wk13,
DmyMi,
Bogdan-Hasanov,
OlhaHoliak,
VyacheslavDzhus,
Yuliya-Fedorenko and
VadymLevkovskyi
SergeyNovitsky
changed the title
| /// <returns>Tuple which contains part of user information (parentId, firstName and lastName.</returns> | ||
| public IEnumerable<(long parentId, string firstName, string lastName)> GetUsersByParents(IEnumerable<long> parentIds); | ||
| /// <returns>List of Parents.</returns> | ||
| public Task<IReadOnlyList<Parent>> GetParentsByIds(IEnumerable<long> parentIds); |
There was a problem hiding this comment.
GetParentsByIdsAsync ?
or, since it is a parents repository: GetByIdsAsync ?
| @@ -36,16 +36,13 @@ public new async Task<Parent> Create(Parent entity) | |||
| } | |||
|
|
|||
| /// <inheritdoc/> | |||
| public IEnumerable<(long parentId, string firstName, string lastName)> GetUsersByParents(IEnumerable<long> parentIds) | |||
| public async Task<IReadOnlyList<Parent>> GetParentsByIds(IEnumerable<long> parentIds) | |||
There was a problem hiding this comment.
Since it is a public method could you please apply argument checks here, like: null check and .Any().
It looks like we can also apply collection element uniqueness check here.
async -> await looks redundant here.
you can use expression body here:
=> db.Parents
.Where(parent => parentIds.Contains(parent.Id))
.ToListAsync();
There was a problem hiding this comment.
Added checks for null argument and if there any result. did not omit async await because of checks, and did not change to expression body too.
| return GetUsersAsync(ratingsDto); | ||
| ratingsDto = await AddUserInformation(ratingsDto).ConfigureAwait(false); | ||
|
|
||
| return ratingsDto; |
There was a problem hiding this comment.
redundant assignment, you can return AddUserInformation(ratingsDto) without awaiting the result here.
| @@ -99,7 +101,9 @@ public async Task<IEnumerable<RatingDto>> GetAllByEntityId(long entityId, Rating | |||
|
|
|||
| var ratingsDto = ratings.Select(r => r.ToModel()); | |||
|
|
|||
| return GetUsersAsync(ratingsDto); | |||
| ratingsDto = await AddUserInformation(ratingsDto).ConfigureAwait(false); | |||
There was a problem hiding this comment.
why do we want to save result into ratingsDto instead of just return the result ?
| @@ -348,17 +354,17 @@ private async Task<bool> EntityExists(long id, RatingType type) | |||
| } | |||
| } | |||
|
|
|||
| private IEnumerable<RatingDto> GetUsersAsync(IEnumerable<RatingDto> ratingDtos) | |||
| private async Task<IEnumerable<RatingDto>> AddUserInformation(IEnumerable<RatingDto> ratingDtos) | |||
| { | ||
| var ratingDtosList = ratingDtos.ToList(); | ||
|
|
||
| var newUsers = parentRepository.GetUsersByParents(ratingDtosList.Select(r => r.ParentId).Distinct()); | ||
| var parents = await parentRepository.GetParentsByIds(ratingDtos.Select(rating => rating.ParentId).Distinct()).ConfigureAwait(false); |
There was a problem hiding this comment.
please extract local variable with parent ids.
| { | ||
| var ratingDtosList = ratingDtos.ToList(); | ||
|
|
||
| var newUsers = parentRepository.GetUsersByParents(ratingDtosList.Select(r => r.ParentId).Distinct()); | ||
| var parents = await parentRepository.GetParentsByIds(ratingDtos.Select(rating => rating.ParentId).Distinct()).ConfigureAwait(false); | ||
|
|
||
| for (int i = 0; i < ratingDtosList.Count; i++) | ||
| { |
There was a problem hiding this comment.
You can use ratingDtoList.ForEach(...) to update elements inside the collection
There was a problem hiding this comment.
I changed it to Select for do not cast IEnumerable to List here.
SergeyNovitsky
force-pushed
the
novitsky/bug_fix_Ratings
branch
from
8d7b995 to
054bcc3
Compare
|
SonarCloud Quality Gate failed. |
No description provided.