Post project completion merge request - ECommerce API#90
Post project completion merge request - ECommerce API#90JJHH17 wants to merge 64 commits intothe-csharp-academy:mainfrom
Conversation
…ry that the product belongs to
Post project completion README commit
Added examples of API endpoints
chrisjamiecarter
left a comment
chrisjamiecarter
left a comment
There was a problem hiding this comment.
Hey @JJHH17 👋,
Excellent work on your Ecommerce API project submission 🎉!
I have performed a peer review. Review/ignore any comments as you wish.
💪 Well done on a great project. I liked your use of pagination and responses in the API, and tried to give tips on how you could improve this. A bit of polish and latest code styles is needed to bring this project up to modern standards, but overall it is a well thought out and structured project.
🟢 Requirements
⭐ You have fulfilled all of the project requirements!
I will go ahead and mark as approved, keep up the excellent work on the next projects! 😊
Best regards,
@chrisjamiecarter 👍
| namespace ECommerceApi.JJHH17.Controllers | ||
| { |
There was a problem hiding this comment.
🟠 Namespaces
💡 As per the academy's code-conventions, use file-scoped namespaces.
➡️ From C# 10, we are able to remove the block/braces and save on level of indentation, optimising space and improving readability.
| namespace ECommerceApi.JJHH17.Controllers | |
| { | |
| namespace ECommerceApi.JJHH17.Controllers; | |
| if (pagination.PageNumber < 1) pagination.PageNumber = 1; | ||
| if (pagination.PageSize < 1) pagination.PageSize = 10; | ||
|
|
||
| var allCategories = _categoryService.GetAllCategories(); |
There was a problem hiding this comment.
🟠 Inefficient
💡 Great job implementing pagination. But ideally you would want to pass the pagination responsibility down to your db context.
💭 Say your database has 10m rows. Currently your api would get 10m rows from the database, hold in memory and then filter down to the first 10 rows (as an example). Imagine the cost, time, and resource implications of this. If using a cloud database provider you would quickly run out of money.
🔧 Instead you should use the database to only get the 10 rows you need, only hold 10 rows in memory, etc, etc.
| if (result == null) { return NotFound(); } | ||
|
|
||
| return Ok(result); |
There was a problem hiding this comment.
🟡 Simplify
💡 Use the ternary conditional operator. condition ? consequent : alternative
💭 e.g. IfThisIsTrue ? DoThis : ElseDoThis
| if (result == null) { return NotFound(); } | |
| return Ok(result); | |
| return result is null | |
| ? NotFound() | |
| : Ok(result); |
| [HttpPost] | ||
| public ActionResult<CreateCategoryDto> CreateCategory(CreateCategoryDto category) | ||
| { | ||
| return Ok(_categoryService.CreateCategory(category)); |
There was a problem hiding this comment.
🟡 REST
💡 Post requests should return a 201 Created response to indicate successful creation and the location of the created resource.
| namespace ECommerceApi.JJHH17.Controllers | ||
| { |
There was a problem hiding this comment.
| namespace ECommerceApi.JJHH17.Controllers | |
| { | |
| namespace ECommerceApi.JJHH17.Controllers; | |
| .Single(); | ||
| } | ||
|
|
||
| public List<GetProductsDto> GetAllProducts() |
| namespace ECommerceApi.JJHH17.Services | ||
| { |
There was a problem hiding this comment.
| namespace ECommerceApi.JJHH17.Services | |
| { | |
| namespace ECommerceApi.JJHH17.Services; | |
| _dbContext = dbContext; | ||
| } | ||
|
|
||
| public List<SaleWithProductsDto> GetAllSales() |
There was a problem hiding this comment.
🟠 README Location
💡 Remember, you are writing a README for your project submission, not the code reviews repository, this file should be in your project submission folder.
📁 CodeReviews.Console.EcommerceApi
┣ 📄 .codacy.yml
┣ 📄 .gitignore
┣ 📁 EcommerceApi.JJHH17
┃ ┗ 📄 EcommerceApi.sln
┃ ┗ 📄 README.md ⬅️ Goes here!
┃ ┗ 📁 EcommerceApi.Api
2 participants
This project:
Project specification and requirements found on: https://www.thecsharpacademy.com/project/18/ecommerce-api