Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create entities crud operations #7

Closed
wants to merge 19 commits into from

Conversation

EmilBulka
Copy link
Collaborator

No description provided.

@Woselko
Copy link
Owner

Woselko commented Jul 31, 2023

Well done @EmilBulka!
Please correct:
1 (OPTIONALLY) Please return StatusCode() method as in other controllers eg:
return StatusCode(StatusCodes.Status403Forbidden, new Response { IsSuccess = false, Message = Common.UserExist});
2. In every controller action please return ActivelyApp.Models.CommonDto.Response not only the string
3. remove comments
image]
4. change Actively/Actively/ActivelyApp/Mappings/PLayerMappingProfile.cs to PlayerMappingProfile.cs
5. I dont like the way that we name folder structure and files
image
In folder called blablablaDTO we should have all files namex xyzDTO.cs
6. CreatePlayerInfo.cs has 3 fields but UpdatePlayerInfo only 2
7. please correct intendation and remove empty lines like here
image
8. I think ForMember() method is useless when property name is the same, please check it and if it not necessary remove it
image
9. I controllers that you created, methods should use not PUT but PATCH HttpMethod explanation here : https://clockworkjava.pl/2021/02/rest-api-post-vs-put-vs-patch/

Copy link
Owner

@Woselko Woselko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rquested changes in main comment

@EmilBulka EmilBulka force-pushed the Create_Entities_CRUD_Operations branch from 86414ee to a8422fe Compare August 20, 2023 20:15
@EmilBulka EmilBulka closed this Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants