submission by 0lcm · Pull Request #79 · the-csharp-academy/CodeReviews.Console.DocumentProcessor

0lcm · 2026-05-18T21:46:02Z

No description provided.

chrisjamiecarter

Hey @0lcm 👋,

Excellent work on your Document Processor project submission 🎉!

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

This is an impressively comprehensive project that demonstrates strong command of ASP.NET Core, Entity Framework Core, and modern C# patterns. The layered architecture (Controllers, Services, Repositories, EF Core) is clean and well-organized. The inclusion of a separate console UI client, import/export support for multiple file formats (CSV, XLSX, XLS, PDF), soft delete via an interceptor, pagination, filtering, and search functionality shows a deep understanding of real-world API development. The README is thorough and well-documented. The use of primary constructors, file-scoped namespaces, collection expressions, and required properties shows you are keeping up with modern C# practices.

Project-Wide Patterns:

Exception handling consistency: Multiple service classes use catch (Exception) { return false; } without logging. Consider injecting ILogger for better debugging.
Disposable resource management: Several IDisposable objects (FileStream, HSSFWorkbook) are used without proper disposal via using.
CS8618 warnings: Non-nullable reference type properties across several model/DTO classes need initialization.
Query string encoding: URL parameters are not encoded, which could break with special characters.

What You Did Well:
⭐ Excellent Architecture: The clean separation into API/Shared/UI projects with distinct layers makes the code testable, maintainable, and follows SOLID principles well.
⭐ EF Core Mastery: Soft delete via SaveChangesInterceptor, many-to-many relationships, query filters, and enum-to-string conversions are all implemented correctly.
⭐ Modern C# Features: Great use of primary constructors, file-scoped namespaces, required properties, collection expressions, and IHttpClientFactory for HTTP client management.
⭐ Factory Pattern for Import/Export: The factory pattern elegantly handles different file formats without coupling to specific implementations.

🔴 Project Submission

💡 Please place all your code inside a single top-level folder named: [ProjectName].[UserName].

👨‍🏫 Example: DocumentProcessor.0lcm

(Even though you are submitting re-used code from the Ecommerce-API project, it should be clear to the reviewer what project is yours).

📁 CodeReviews.Console.DocumentProcessor. ⬅️ Forked repo
 ┣ 📄 .codacy.yml
 ┣ 📄 .gitignore
 ┣ 📁 DocumentProcessor.0lcm ⬅️ Top-level folder
 ┃  ┗ 📄 Project.sln
 ┃  ┗ 📄 README.md
 ┃  ┗ 📁 ECommerce.API

💭 This helps me clearly identify your work and confirms you can work within community repository guidelines.

🔧 Next Steps

Please fix any 🔴 items, as they block approval.
Commit and push your changes, then leave me a comment when done.
Feel free to reach out if you have any questions or want to discuss further 🆘.

Thanks,
@chrisjamiecarter 👍

chrisjamiecarter · 2026-07-05T16:47:55Z

+    }
+
+    [HttpGet]
+    public async Task<ActionResult<PagedResponse<Item>>> GetItemsAsync([FromQuery] PaginationParams paginationParams)


🟠 Type Mismatch in Action Return Type

💡 The method signature declares ActionResult<PagedResponse<Item>> but the actual response comes from the service which returns PagedResponse<ItemDto>. While this compiles, it will cause incorrect OpenAPI/Swagger documentation.

chrisjamiecarter · 2026-07-05T16:48:44Z

+    }
+
+    [HttpDelete]
+    public async Task<IActionResult> DeleteSaleAsync([FromQuery] int saleId)


🟡 Unconventional DELETE Route

💡 The delete action uses [HttpDelete] with no route template and accepts [FromQuery] int saleId. The standard REST convention is to use a route parameter: [HttpDelete("{saleId:int}")] and int saleId without [FromQuery]. This is more idiomatic for ASP.NET Core APIs.

chrisjamiecarter · 2026-07-05T16:49:15Z

+    app.UseSwaggerUI();
+}
+
+using (var scope = app.Services.CreateScope())


🟡 Using Statement vs Using Declaration

💡 You are using the using (var scope = ...) statement pattern. The C# Academy guidelines prefer the using declaration pattern:
csharp using var scope = app.Services.CreateScope();

chrisjamiecarter · 2026-07-05T16:49:42Z

+app.UseHttpsRedirection();
+app.UseAuthorization();
+app.MapControllers();
+app.UseExceptionHandler(error => error.Run(async context =>


🟡 Error Handler Placement

💡 The UseExceptionHandler middleware is registered after MapControllers(). For exception handling middleware to catch controller exceptions, it should be registered earlier in the pipeline (right after the environment check).

chrisjamiecarter · 2026-07-05T16:50:14Z

+
+namespace ECommerce.UI.Helpers;
+
+internal class UiHelper(ITagService tagService)


🟠 Unused Constructor Parameter

💡 The tagService parameter in the UiHelper constructor is injected but never used (build warning CS9113). Either remove the parameter and its DI registration, or use the service within the class.

chrisjamiecarter · 2026-07-05T16:50:35Z

+                DisplayWarning("An error has occurred with one or more of the arguments you have entered, " +
+                               "please check that any details you enter are correct before trying again.");
+        }
+        else if (ex is NotSupportedException notSupportedException)


🟠 Empty Catch Block

💡 The NotSupportedException is caught with an empty handler body. This silently swallows the exception. Either log it, add a user-facing message, or remove the catch clause.

chrisjamiecarter · 2026-07-05T16:51:19Z

+
+namespace ECommerce.API.Services.Import_Export;
+
+public class PdfService(IConfiguration configuration) : IExportService


🟠 Unused Constructor Parameter

chrisjamiecarter · 2026-07-05T16:51:58Z

+            await repo.DeleteItemAsync(item);
+            return true;
+        }
+        catch (Exception)


🟠 Swallowing Exceptions Without Logging

chrisjamiecarter · 2026-07-05T16:52:36Z

+    public SeedData GetSeedData(IEnumerable<string> filePaths)
+    {
+        var filePath = filePaths.First();
+        var stream = new FileStream(filePath, FileMode.Open, FileAccess.Read);


🔴 Potential Resource Leak

💡 The FileStream and HSSFWorkbook objects implement IDisposable but are never disposed. Wrap them in using statements:
csharp using var stream = new FileStream(filePath, FileMode.Open, FileAccess.Read); using var workbook = new HSSFWorkbook(stream);

chrisjamiecarter · 2026-07-05T16:57:50Z

+        sb.Append($"{baseUrl}?PageNumber={pageNumber}&PageSize={pageSize}");
+
+        if (!string.IsNullOrWhiteSpace(searchTerm))
+            sb.Append($"&SearchTerm={searchTerm}");


🟡 SearchTerm in Query String Not URL-Encoded

💡 When searchTerm contains special characters like spaces or ampersands, the query string will break. Use URL-encoding:
csharp sb.Append($"&SearchTerm={Uri.EscapeDataString(searchTerm)}");
(This also applies to Genre on line 19 and SearchTags on line 24)

0lcm · 2026-07-05T23:40:39Z

Hi @chrisjamiecarter, I just pushed the requested changes so tell me if there are any other problems, and thank you for reviewing my project!

chrisjamiecarter

Hey @0lcm 👋,

Great uptdates to work on your Document Processor project submission 🎉!

🟢 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 👍

chrisjamiecarter · 2026-07-06T15:24:03Z

 namespace ECommerce.API.Services.Import_Export;

-public class PdfService(IConfiguration configuration) : IExportService
+public class PdfService() : IExportService


Rather than leave an empty constructor, change to:
public class PdfService : IExportService

chrisjamiecarter · 2026-07-06T15:25:03Z

 namespace ECommerce.UI.Helpers;

-internal class UiHelper(ITagService tagService)
+internal class UiHelper()


internal class UiHelper

chrisjamiecarter

Hey @0lcm 👋,

Great updates to your Document Processor project submission 🎉!

🟢 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 👍

submission commit

0006cbb

chrisjamiecarter suggested changes Jul 5, 2026

View reviewed changes

fix requested changes

3b5643c

chrisjamiecarter reviewed Jul 6, 2026

View reviewed changes

chrisjamiecarter approved these changes Jul 6, 2026

View reviewed changes


		namespace ECommerce.UI.Helpers;

		internal class UiHelper(ITagService tagService)


		namespace ECommerce.API.Services.Import_Export;

		public class PdfService(IConfiguration configuration) : IExportService

Uh oh!

Conversation

0lcm commented May 18, 2026

Uh oh!

chrisjamiecarter left a comment

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!

Choose a reason for hiding this comment

Uh oh!

0lcm commented Jul 5, 2026

Uh oh!

chrisjamiecarter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

chrisjamiecarter left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisjamiecarter left a comment •

edited

Loading