Submitted for review by RyanW84 · Pull Request #92 · the-csharp-academy/CodeReviews.Console.EcommerceApi

RyanW84 · 2026-01-11T21:27:08Z

Codacy errors ignored, they break the app if changed

chrisjamiecarter

Excellent work on your ECommerce API project submission 🎉!

I have performed a peer review. Review/ignore any comments as you wish.

Overview

This is an impressive, production-ready API implementation that demonstrates sophisticated understanding of ASP.NET Core patterns. The project showcases:

Clean Architecture: Well-separated concerns with Controllers → Services → Repositories → Data layer
Soft Delete Pattern: Elegant implementation using global query filters and BaseEntity
Performance Optimizations: Response caching, output caching, response compression, compiled queries
Documentation Excellence: Comprehensive README, XML docs, Swagger/Scalar integration
Testing Strategy: Unit tests with proper Arrange-Act-Assert patterns (61 passing tests!)

Suggestions for Improvement

Project Structure 🟠 - Code in Root Instead of Username Folder
The project files are in the repository root instead of a proper project folder containing the username. This violates the requirement that all student code must be inside a single top-level folder named [ProjectName].[Username]. I think at this point in your journey you should be following this basic step during project submission.

📁 CodeReviews.Console.EcommerceApi ⬅️ Forked repo
 ┣ 📄 .codacy.yml
 ┣ 📄 .gitignore
 ┣ 📁 EcommerceApi.RyanW84 ⬅️ Top-level folder *Missing!
 ┃  ┗ 📄 EcommerceApi.sln
 ┃  ┗ 📄 README.md
 ┃  ┗ 📁 EcommerceApi.Api
 ┃  ┗ 📁 EcommerceApi.Ui

Also, as you have included tests, a common standard is to include a src and a tests folder. So that would slightly change your structure:

📁 CodeReviews.Console.EcommerceApi ⬅️ Forked repo
 ┣ 📄 .codacy.yml
 ┣ 📄 .gitignore
 ┣ 📁 EcommerceApi.RyanW84 ⬅️ Top-level folder *Missing!
 ┃  ┣ 📄 EcommerceApi.sln
 ┃  ┣ 📄 README.md
 ┃  ┣ 📁 src ⬅️ src-level folder *New!
 ┃  ┃  ┗ 📁 EcommerceApi.Api
 ┃  ┃  ┗ 📁 EcommerceApi.Ui
 ┃  ┣ 📁 tests
 ┃  ┃  ┗ 📁 EcommerceApi.Api.Tests
 ┃  ┃  ┗ 📁 EcommerceApi.Ui.Tests

Integration Tests Broken 🟠 - The WebApplicationFactory<Program> cannot access the internal Program class. Add public partial class Program { } to make the entry point public for testing.
Namespace Inconsistency 🟠 - Controllers use file-scoped namespaces (namespace X;) while Services and Repositories use traditional block namespaces (namespace X { }). Choose one style and apply consistently.
XML Doc Placement 🟠 - In CategoriesController and SalesController, the class documentation appears after attributes instead of before them.
Required Properties Pattern 🟡 - Using required string Name { get; set; } = null!; suppresses null warnings without providing safety. Remove the = null! initializers.
Generic Exception Handling 🟡 - Services catch Exception broadly. Consider whether the global exception middleware could handle some of these cases, reducing duplicated try/catch blocks.

I will go ahead and mark as approved, keep up the excellent work on the next projects! 😊

Thanks,
@chrisjamiecarter 👍

chrisjamiecarter · 2026-02-16T13:58:34Z

Controllers/CategoriesController.cs

+[ApiController]
+[Route("api/[controller]")]
+[Route("api/v1/categories")]
+/// <summary>
+/// Category CRUD API endpoints.
+/// Provides create, read (by id/name), update, delete, and soft-delete restore operations for <see cref="Category"/>.
+/// Responses are wrapped in the project's standard API response DTOs and follow the same error mapping conventions.
+/// </summary>
+public class CategoriesController : ControllerBase


🟠 XML Documentation Placement

💡 The class-level XML documentation comment is placed after the [ApiController] and [Route] attributes. Per standard conventions, documentation comments should precede all attributes:

Suggested change

[ApiController]

[Route("api/[controller]")]

[Route("api/v1/categories")]

/// <summary>

/// Category CRUD API endpoints.

/// Provides create, read (by id/name), update, delete, and soft-delete restore operations for <see cref="Category"/>.

/// Responses are wrapped in the project's standard API response DTOs and follow the same error mapping conventions.

/// </summary>

public class CategoriesController : ControllerBase

/// <summary>

/// Category CRUD API endpoints.

/// Provides create, read (by id/name), update, delete, and soft-delete restore operations for <see cref="Category"/>.

/// Responses are wrapped in the project's standard API response DTOs and follow the same error mapping conventions.

/// </summary>

[ApiController]

[Route("api/[controller]")]

[Route("api/v1/categories")]

public class CategoriesController : ControllerBase

chrisjamiecarter · 2026-02-16T13:59:14Z

Controllers/SalesController.cs

+[ApiController]
+[Route("api/[controller]")]
+[Route("api/v1/sales")]
+/// <summary>
+/// Sales API endpoints.
+/// Supports creating and managing sales, including read operations that can optionally include historical (soft-deleted) products.
+/// </summary>
+public class SalesController : ControllerBase


🟠 XML Documentation Placement

💡 The class-level XML documentation comment is placed after the [ApiController] and [Route] attributes. Per standard conventions, documentation comments should precede all attributes:

Suggested change

[ApiController]

[Route("api/[controller]")]

[Route("api/v1/sales")]

/// <summary>

/// Sales API endpoints.

/// Supports creating and managing sales, including read operations that can optionally include historical (soft-deleted) products.

/// </summary>

public class SalesController : ControllerBase

/// <summary>

/// Sales API endpoints.

/// Supports creating and managing sales, including read operations that can optionally include historical (soft-deleted) products.

/// </summary>

[ApiController]

[Route("api/[controller]")]

[Route("api/v1/sales")]

public class SalesController : ControllerBase

chrisjamiecarter · 2026-02-16T14:01:58Z

Controllers/SalesController.cs

+    // GET /api/v1/sales/history
+    // Noun-based alternative to "with-deleted-products".
+    [HttpGet("history")]
+    [ResponseCache(Duration = 30, Location = ResponseCacheLocation.Any)]
+    public Task<IActionResult> GetHistory(CancellationToken cancellationToken) =>
+        GetAllWithDeletedProducts(cancellationToken);


🔵 Noun-Based Route Alternative Comment

💡 The comment // Noun-based alternative to "with-deleted-products". is helpful, but consider if maintaining both endpoints is necessary. Having both /with-deleted-products and /history could confuse API consumers. Consider deprecating one or documenting which is preferred.

chrisjamiecarter · 2026-02-16T14:02:54Z

Data/Models/Product.cs

+    /// <summary>
+    /// Display name shown in lists and detail views.
+    /// </summary>
+    public required string Name { get; set; } = null!;


🟡 Required Properties with null! Initialization

💡 Using required string Name { get; set; } = null!; defeats the purpose of the required modifier. The null! suppresses null warnings but doesn't provide compile-time safety. Consider removing the = null! initializer to ensure consumers must set the value.

chrisjamiecarter · 2026-02-16T14:03:34Z

Services/ProductService.cs

+namespace ECommerceApp.RyanW84.Services
+{


🟠 Block Namespace Instead of File-Scoped

💡 This file uses traditional block namespaces while Controllers use file-scoped namespaces. For consistency across the codebase, prefer file-scoped namespaces.

Suggested change

namespace ECommerceApp.RyanW84.Services

{

namespace ECommerceApp.RyanW84.Services;

chrisjamiecarter · 2026-02-16T14:21:54Z

Controllers/ProductController.cs

+[ApiController]
+[Route("api/[controller]")]
+[Route("api/products")]
+[Route("api/v1/products")]
+public class ProductController(IProductService productService) : ControllerBase


🟡 Redundant Route Attributes

❓ Why do you have duplication in your routes? Why not just keep only the versioned route: [Route("api/v1/[controller]")]

chrisjamiecarter · 2026-02-16T14:25:37Z

Repositories/ProductRepository.cs

+                "price" => descending
+                    ? query.OrderByDescending(p => p.Price)
+                    : query.OrderBy(p => p.Price),
+                "createdat" => descending


❓ Why createdat and not createdate? Also in CategoryRepository.

chrisjamiecarter · 2026-02-16T14:32:03Z

tests/ArchitectureTests/SmokeTests.cs

+public class SmokeTests
+{
+    [Fact]
+    public void True_is_true()


❓ What benefit does this bring?

chrisjamiecarter · 2026-02-16T14:33:04Z

tests/ECommerceApp.UnitTests/ConsoleClient/TableRendererDynamicPagingTests.cs

+namespace ECommerceApp.UnitTests.ConsoleClient;
+
+[Collection(SpectreConsoleCollection.Name)]
+public class TableRendererDynamicPagingTests


❓ What is the purpose of these tests? It seems you are testing Spectre.Console. If so, I would leave that to the owners of that library and not worry about it.

chrisjamiecarter · 2026-02-16T14:33:33Z

tests/ECommerceApp.UnitTests/CategoryValidatorTests.cs

+    private readonly CategoryValidator _validator = new();
+
+    [Fact]
+    public void Validate_WithValidCategory_ShouldSucceed()


🟢 Good Test Structure

💡 Tests follow Arrange-Act-Assert pattern with clear naming. Excellent test structure!

RyanW84 added 3 commits January 11, 2026 21:02

submitted for review

09321fa

submitted for review

7c1914f

codacy config updated

d3a8b1e

chrisjamiecarter approved these changes Feb 16, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Submitted for review#92

Submitted for review#92
RyanW84 wants to merge 3 commits intothe-csharp-academy:mainfrom
RyanW84:main

RyanW84 commented Jan 11, 2026 •

edited

Loading

Uh oh!

chrisjamiecarter left a comment

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

chrisjamiecarter Feb 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	namespace ECommerceApp.RyanW84.Services
	{
	namespace ECommerceApp.RyanW84.Services;

Conversation

RyanW84 commented Jan 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisjamiecarter left a comment

Choose a reason for hiding this comment

Overview

Suggestions for Improvement

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RyanW84 commented Jan 11, 2026 •

edited

Loading