Achievements #669
Achievements #669
Conversation
| [Route("api/v{version:apiVersion}/[controller]/[action]")] | ||
| public class AchievementController : ControllerBase | ||
| { | ||
| private readonly IAchievementService service; |
There was a problem hiding this comment.
Better use achievementService name instead service, I think
| { | ||
| public class AchievementTeacher | ||
| { | ||
| public long Id { get; set; } |
There was a problem hiding this comment.
Please, add AchievementTeacher : IKeyedEntity<long> and the same for AchievementType
|
|
||
| public virtual Achievement Achievement { get; set; } | ||
|
|
||
| [Required(ErrorMessage = "Title is required")] |
There was a problem hiding this comment.
I guess that ErrorMessage in these types in DataAccessLayer must be irrelevant. The same in Achievement, AchievementType.
| [ProducesResponseType(StatusCodes.Status500InternalServerError)] | ||
| public async Task<IActionResult> Create(AchievementCreateDTO achievementDto) | ||
| { | ||
| if (!ModelState.IsValid) |
There was a problem hiding this comment.
The [ApiController] attribute makes model validation errors automatically trigger an HTTP 400 response. Consequently, the following code is unnecessary in an action method. The same for other methods here.
| /// <inheritdoc/> | ||
| public async Task<AchievementDto> Create(AchievementCreateDTO dto) | ||
| { | ||
| logger.LogInformation("Achievement creating was started."); |
There was a problem hiding this comment.
Please, check dto argument on null here and in Update() method
| ILogger<AchievementService> logger, | ||
| IStringLocalizer<SharedResource> localizer, | ||
| IMapper mapper, | ||
| OutOfSchoolDbContext context) |
There was a problem hiding this comment.
I think it's better to create a related repository rather than pass db context in order to separate repository/service responsibility. Ultimately, with this change you will able to avoid duplicate queries for your methods.
|
|
||
| logger.LogInformation($"Achievement with Id = {id} succesfully deleted."); | ||
|
|
||
| return Result<AchievementDto>.Success(mapper.Map<AchievementDto>(entity)); |
There was a problem hiding this comment.
You wrapped result in custom Result<AchievementDto> but:
- It contains only id to return.
- You don't define Failed result in this code.
- You don't use benefits of such result in your controller action.
I'd better delete this wrapper.
Best way in your case is just make public async Task Delete(Guid id). And maybe I'd extend an exception argument to upstream DbUpdateException. Then if it deletes, you'll be able to send Ok response in controller action (you already has got Id there as parameter if you wanna send it in message) else you'll get an exception in service.
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public async Task<AchievementDto> Update(AchievementCreateDTO dto) |
There was a problem hiding this comment.
Just a comment, not a call to actions. AchievementCreateDTO as parameter in Update method.
| achievement.Title = dto.Title; | ||
| achievement.AchievementDate = dto.AchievementDate; | ||
| achievement.WorkshopId = dto.WorkshopId; | ||
| achievement.Children.RemoveAll(x => dto.ChildrenIDs.IndexOf(x.Id) < 0); |
There was a problem hiding this comment.
How about achievement.Children.RemoveAll(x => !dto.ChildrenIDs.Contains(x.Id));?
And also check achievement.Teachers.RemoveAll() method below.
| achievement.Children.RemoveAll(x => dto.ChildrenIDs.IndexOf(x.Id) < 0); | ||
| achievement.Children.AddRange(context.Children.Where(w => dto.ChildrenIDs.Contains(w.Id)).ToList()); | ||
|
|
||
| achievement.Teachers.RemoveAll(x => dto.Teachers.IndexOf(x.Title) < 0); |
There was a problem hiding this comment.
achievement.Teachers.RemoveAll(x => !dto.Teachers.Contains(x.Title));
| achievement.Teachers.RemoveAll(x => dto.Teachers.IndexOf(x.Title) < 0); | ||
| foreach (var Teacher in dto.Teachers) | ||
| { | ||
| if (achievement.Teachers.Find(x => x.Title == Teacher) is null) |
There was a problem hiding this comment.
- name
dto.Teacherswhich containsstrings(that in your case is equal toTitle) looks bad - You could rewrite something like this.
var exceptTeachers = dto.Teachers
.Where(p => achievement.Teachers.All(x => !string.Equals(x.Title, p, StringComparison.InvariantCultureIgnoreCase)));
Or with Except
var exceptTeachers = dto.Teachers
.Except(achievement.Teachers.Select(x => x.Title));
Then
foreach (var teacherId in exceptTeachers)
{
achievement.Teachers.Add(new AchievementTeacher { Title = dto.Title, Achievement = achievement });
}
Should compare strings by Equals() method
|
And also I haven't seen your new migration considering new Db sets (if you remember, it's Ok :)) |
DmyMi
force-pushed
the
mhrytc/achievement
branch
from
9e3b94c to
be2c160
Compare
DmyMi
force-pushed
the
mhrytc/achievement
branch
from
be2c160 to
9d5cccc
Compare
|
SonarCloud Quality Gate failed. |
No description provided.