ardalis / ardalis/RiverBooks

Reader Feedback

Open
#2 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C#
Stars
133
Forks
42
Avg merge
23h 6m
Merged PRs (30d)
3

Description

This might need split into several issues, but for now tracking them all here:

```
Your name:
Antoni Bélanger

Email:
()

Subject:
Getting Started: Modular Monolith

Message:
Hi,

I've just completed the Getting Started: Modular Monolith course and had a few questions and comments.

First, let's start with great job! I liked the course and I'm gonna start on the deep dive soon.

Question: BookDetailsHandler
In the BookDetailsHandler you injected the IBookService, why not use the IBookRepository?

Using the book service adds an unnecessary layer that will add conversions, i.e. Book -> BookDto -> BookDetailsResponse, instead of Book -> BookDetailsResponse
Also depending on how that additional layer handles the NotFound case, this could impact the handler. In this case the handler wants to return NotFound, but the service could throw an exception disrupting the desired behaviour.
That seemed like an honest mistake, but I might be missing something.

Comment: CartItem
In CartItem, you added a constructor for EF Core with a comment that it's why.
Fortunately that was not needed as the problem was the configuration, though you let it there in the course.
It would be nice to point out that the CartItem is a Domain object and really should not have anything to do with EF Core. Even an empty public constructor, as this could be use and result in invalid domain objects.

Comment: Tests
To me tests are fundamentals to development and shipping code that does not have automated tests that proves it works as expected is not professional.
I understand that in the context of a course that you won't necessarily do that, as it would add a significant amount of time on something that is not the focus of the course.
However, I do like it when the teacher takes time to point out how important it is and that in a real application you should have automated tests. Just so it gets hammered down little bit on the students and make it obvious that this part is missing.
It's a little thing, but so often feels unimportant when learning that people tend to en up thinking that it's not important. So essentially I just wish it was mentioned more.

Comment: Source code dependency
Perhaps mention a little bit more about clean architecture.
What I mean by that is that you can categorize any of the class into "Domain", "Application", "Infrastructure" or "Presentation".
I understand that the course is not about clean architecture and that you're not gonna go into details here.

The point I'd like to be made a little clearer for the students is about source code dependencies.
A common mistake I see is that people use object from the infrastructure in application objects, which is very easy to do in modular monolith as they are usually in the same project. But there are many reasons why you don't want to do that.
Sadly I don't have a solution on how to actually get that point across better in a course without taking a significant amount time.

That's it off the top my head and that was already pretty long. I hope that it will be useful and keep up the good work.
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Review the course material covering BookDetailsHandler, CartItem, tests, and source-code dependencies; no files or test entry points are named in the issue. First separate the feedback into actionable documentation issues, then define the specific course changes and confirm that each concern is addressed.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
documentation
Issue type
Documentation
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.