Jeremy Caney (28) [Avatar] Offline
#1
Steven, Mark—

I ended up with a lot of feedback on Chapter 3. Much of this is more of the same—minor typos, suggestions on wording to improve flow, &c. In addition, however, I also have quite a bit of feedback on 3.1.2 Building an independent Domain Model, specifically.

That is a challenging section because it's fairly architecturally complicated, and thus you end up introducing a number of interfaces and classes before it's entirely intuitive how they fit together (even despite the figures that map it out). To help mitigate this, I recommend a number of clarifications which, while not strictly necessary, help pair the abstract language of design patterns to the concrete references of the implementation example. Sometimes this feels a bit redundant, but I think it will help bridge the reader's understanding between the concept and the implementation, and especially if they haven't fully internalized the design patterns yet. Further, I also suggest some changes to the identifier names to better articulate what each represents.

Typos
Most of these would likely be picked up by you or your copy editors on a future pass, but I figured I'd flag them while I'm at it. These are minor typos that are easy to fix.

  • 3: Throughout this chapter, you may want to standardize between "Data Access library" and "Data Access Layer", as well as between "Domain Model" and "Domain library" for consistency; these are currently used interchangeably.

  • 3: In the second paragraph, it should read, "As important as it is…" (currently, it's missing the word "as").

  • 3: In the Note, "discourage" should be "encourage".

  • 3: The last sentence should read, "Let's start with a short recap of Mary's application, how we will approach the rebuild and what describe the desired result is…"

  • 3.1: In the second paragraph after Figure 3.2, "separation" should be "separating" as in "We will try to do a better job in separating the application's concerns."

  • 3.1: After Figure 3.4, it should read "…we'll look take a look at a slightly more advanced version…"

  • 3.1: After Figure 3.4, "This is often the user interface, as is in our commerce application" should be "as it is in" or "as in".

  • 3.1.1: In the paragraph after Listing 3.2, "casted" should read "cast".

  • 3.1.1: In the second Note, in the fourth sentence, either the first "them" should be replaced with "it" or the second "them" should be removed; e.g., "This makes them safe to new them up inside our code…"

  • 3.1.1: In the paragraph after Listing 3.4, it should read, "In its public constructor…"

  • 3.1.2: In the first paragraph, "external entry point" should be "external entry points" since it's talking about multiple interfaces.

  • 3.1.2: Before the second Note, it should read, "…we allow the retrieval of this information to become…"

  • 3.1.2: Before the second Note, it should read, "…without the HomeController from having to worry about this."

  • 3.1.2: In the Dependency Inversion Principle callout, it should read, "This brings us back to the BCL's IPrincipal interface."

  • 3.1.2: In the bullet list after the Dependency Inversion Principle callout, "ProductRepository" should be "SqlProductRepository".

  • 3.1.2: After Figure 3.8, it should read, "…that is independent of the Data Access layer, which we still need to create." (It currently reads, "that".)

  • 3.1.3: In the To Seam or not to Seam callout, it should be "the IProductRepository".

  • 3.1.3: In the To Seam or not to Seam callout, it should read, "This was done transparently and centrally." (As "This was done… centralized" doesn't make sense.)

  • 3.1.4: The Figure 3.9 caption, it should read, "…has an interface incompatible with the Target."

  • 3.1.4: In the caption beneath Figure 3.10, it should read, "…it prevents the client from having to depend upon the HttpContext." It currently says, "…from requiring to depend upon…"

  • 3.1.4: In the paragraph after Figure 3.10, it should read, "…where HomeController is fed by a ProductService instance, which itself is constructed using a SqlProductrepository and an AspNetUserContextAdapter." ("Itself" isn't required, but reads better.)

  • 3.1.4: In the paragraph after Figure 3.13, it should read, "Because we discuss how the construction of such an object graph is plugged into the…"

  • 3.2.1: The last sentence of the first paragraph should read, "Figure 3.4 illustrated how dependencies are being connected that."

  • 3.2.2: In Figure 3.13, FeaturedProductViewModel should be FeaturedProductsViewModel, AspNetUserContextAdapter should be under the Data Access library, not the User Interface library, and the connection string should also be listed under the Data Access library.

  • 3.2.2: In the bullet list after Figure 3.13, it should read, "…as an application setting in the application's configuration file."

  • 3.2.2: In the About DI Containers callout, "prevented" should be "presented"—hopefully you're not preventing knowledge in part 2 and 3!

  • 3.2.2. The last paragraph should start with, "In this chapter…"

  • 3.3 In the second bullet, it should read, "The use of View Models or DTOs can simplify the view, since since the incoming data is shaped especially for the view."

  • 3.3 In the eighth bullet, "programming" is misspelled as "pogramming".


  • Punctuation and Formatting
    The items in this section aren't necessarily typos—or even incorrect, per se—but are minor changes, primarily to the punctuation and formatting, that might improve the flow by e.g. joining related ideas, separating out concepts, or emphasizing an identifier as code.

  • 3: In the third paragraph, consider "To summarize, loose coupling…" so that there's only one colon in the sentence.

  • 3: In the Note, it should read, "…slow, step-by-step refactorings…"

  • 3.1.2: In the Interfaces or abstract classes? callout, there's an unnecessary comma in, "…we typically prefer interfaces over abstract classes, because:"

  • 3.1.2: In the Dependency Inversion Principle callout, I'd change: "The principle states that higher-level modules in our applications should not depend on lower-level modules; instead both modules should depend on Abstractions."

  • 3.1.2: In the Dependency Inversion Principle callout, I'd change: "This does mean, however, that at some place in the application, we will have to…" (though you could also make an argument for putting a comma after "that" instead).

  • 3.1.2: After the Dependency Inversion Principle callout, there is an extraneous comma: "At this stage, there's only a single concept represented, namely, a Product."

  • 3.1.4: In the caption under Listing 3.10, there is a missing comma: "This allows the client code to be simplified and, in this particular case, it prevents…"

  • 3.2.2: In the About DI Container callout, it should read, "This is deliberate, because, as we explained in Chapter 1…"

  • 3.2.2. In the About DI Containers, in the Warning, it should read, "Don't expect a DI Container to magically change tightly-coupled code into loosely-coupled code."

  • 3.3 In the first bullet, it should read, "Refactoring existing applications towards a more maintainable, loosely-coupled design, is hard. Big rewrites, on the other hand, are even more costly and much more risky."

  • 3.3 In the eighth bullet, place commas around the "however", i.e. "Programming to interfaces, however, doesn't mean that…"


  • Readability Suggestions
    The following are areas I found myself tripping over the wording, possibly due to repetition of words, run-on sentences, or simply words that, while correct, interfered with the readability of the content. These are fairly subjective, and more subject to stylistic preferences, so take them with a grain of salt. At the same time, these are also the trickiest issues for me to pick up when proof-reading my own writing, so I figured I'd document them.

  • 3: In the Note, replace "and neither" with "nor"; it flows better.

  • 3: In the last sentence, consider removing "…on the end of this chapter". If you keep it, it would read better earlier in the sentence, such as "…and, at the end of the chapter, what the desired result is."

  • 3.1: In the paragraph after Figure 3.2, I'd reword the last sentence to "We will also use Method Injection and a Composition Root, which we will discuss them as we go."

  • 3.1: In the second paragraph after Figure 3.4, I recommend simplifying the sentence to, "After the HomeController is constructed, MVC will invoke its Index method, which will call into the ProductService, which in turn calls into the SqlProductRepository."

  • 3.1.1: In the paragraph after Listing 3.3, consider merging the two sentences to reduce repetition: "The use of View Models or other Data Transfer Objects simplifies the viewwhich is good, because views are much harder to test."

  • 3.1.1: In the second Note, remove the second instance of "interesting"; it is repetitive with the previous sentence, and can be safely removed without changing the message.

  • 3.1.1: I recommend the following changes: "From the perspective of the HomeController, however, ProductService is a Volatile Dependency, because it is a Dependency that doesn't yet exist, and is still in development."

  • 3.1.1: In the next sentence, I'd change, "If we want to be able to test the HomeController in isolation, develop the ProductService in parallel, or want to be able to replace or intercept it in the future, we should introduce an Abstraction." The "want to be able to" is redundant.

  • 3.1.1: After the Composition Root callout, replace "…and that's the whole point" with e.g., "…and that's precisely why we do it" in order to avoid repetition of "the whole point" from the previous paragraph ("Yes, it does—and that's the whole point").

  • 3.1.2: In the Interfaces or abstract classes? callout, consider: "This doesn't mean though that we, as authors, don't have a preference for one over the other. We do, in fact.”

  • 3.1.2: After Listing 3.7, in the Note, consider "…we allow the whole library to be replaced" to emphasize the significance of that concept.

  • 3.1.2: In the Entity vs. DTO callout, consider "…but it simply means that…"

  • 3.1.2: In Listing 3.9's annotation #2, consider e.g., "The repository and userContext dependencies are used, respectively, to pull a list of discounts and to apply a discount for each featured product." This acknowledges that the repository has a separate function than the userContext (even if they're used in conjunction with one another).

  • 3.1.2: Consider, "Besides that, information about the user on whose behalf the request is running, is contextual information."

  • 3.1.2: Consider, "We don't want every Controller to be responsible for gathering this information; that would be repetitive…"

  • 3.1.2: In the second Note, I'd recommend "This is typically information…" over "This typically is information…" as it flows a bit better.

  • 3.1.2: In the Dependency Inversion Principle callout, I'd say "…and DI how we would like to accomplish it."

  • 3.1.2: In the Dependency Inversion Principle callout, I'd change: "There is, however, another interesting part of the Dependency Inversion Principle that many developers don't know about: not only does this principle…"

  • 3.1.2: In the Dependency Inversion Principle callout, I'd change: "They are designed in a way that works best for the Domain layer, even if that means that it becomes harder for…"

  • 3.1.2: In the bullet list after the Dependency Inversion Principle callout, I'd clarify, "We'll take a look at this in the next section as well."

  • 3.1.2: After Figure 3.8, I'd say, "We succeeded in making our Domain Model compile" instead of "We succeeded to make our Domain Model compile"; it flows easier.

  • 3.1.3: After Listing 3.10, change "In Mary’s application, the Product entity was also used as a Domain object, although it was defined in the Data Access layer." This disambiguates the next sentence, "This is no longer the case", by making it clear that it refers to the second part of the sentence and not the first.

  • 3.1.3: In the first Note, replace "If we come at a later stage to the conclusion that…" with simply, "If we later conclude that…"

  • 3.1.3: In the To Seam or not to Seam callout, consider "considering it requires a database to be setup and configured" instead of "a set up and configured database".

  • 3.1.3: In the To Seam or not to Seam callout, reword to e.g., "By disallowing consumers from direct access to the DbContext, they were forced to use the built-in, row-based filtering."

  • 3.1.3: In Listing 4.11's annotation #2, it should read, "…and used later on in the OnConfiguring method to set up the…"

  • 3.1.4: In the first paragraph, try "This information is relayed using cookies…" to avoid repetition of "passed on".

  • 3.1.4: In the first paragraph, it would read better as, "How we need to retrieve the identity of the current user…"

  • 3.1.4: Start the second paragraph off with, "In other words, the implementation of our IUserContext…" to avoid the repetition of "This means that…" from the previous paragraph.

  • 3.1.4: I'd recommend breaking up the second sentence in the second paragraph into two sentences to avoid the long parenthetical.

  • 3.1.4: In the second paragraph, it would be less wordy if you removed the words "used". This occurs twice in, "…the used application framework…"

  • 3.1.4: In the second paragraph, consider "Since a Composition Root is established as close as possible…"; with the word "defined" it's ambiguous as to whether you're talking about where the Composition Root is defined, or what its definition is.

  • 3.1.4: In the second paragraph, consider "…this makes the Composition Root specific to the running application and the application framework, and thus an ideal place for our IUserContext implementation." This avoids the repetition of "makes".

  • 3.1.4: In the first paragraph after Listing 3.12, consider "HttpContextAccessor is a component specified by the ASP.NET Core framework, which allows to access to the HttpContext of the current request, much like we were able to access the request's HttpContext in ASP.NET 'classic' using HttpContext.Current."

  • 3.1.4: In the paragraph under Figure 3.12, it would read easier as, "ProductService in turn is injected into…"

  • 3.2.2: In the Figure 3.13 caption, consider "All classes and interfaces are shown as well as their relationships to one another."

  • 3.2.2: In the About DI Container callout, the sentence starting with, "We build applications both with and without DI Container…" is quite long, and would benefit from being broken down into two, maybe even three sentences. E.g., "We build applications both with an without DI Containers and you should be able to do so as well. And, yet, we feel it would be counterproductive if you started using a DI Container without the knowledge presented in part 2 and 3. On the other hand, after you understand the principles and practices, using a DI Container primarily consists of getting acquainted with its API." (I made a few other wording suggestions here while I was at it.)

  • 3.2.2: In the About DI Container callout, consider, "When the first edition of this book came out, we used DI Containers exclusively in all of the applications we built. Although we knew that applying DI without a DI Container was possible, we thought that it was never really practical."

  • 3.2.2. In the About DI Containers, consider removing "This idea was reflected in the first edition." This is implicit due to the introduction, which states "When the first edition of this book came out…"

  • 3.2.2. In the last paragraph, it might read better as, "…and introduced Method Injection and the Composition Root as patterns related to DI."

  • 3.3 In the tenth bullet, consider, "From a more general development perspective, as authors, we do typically prefer interfaces over abstract classes." It flows a bit better.


  • Content Suggestions
    The following are more invasive changes related to organization and order of content, technical details, &c.

  • 3.1: In Figure 3.3, UserContextAdapter, should be under the Data Access library, not the User Interface library. You may also want to account for the connection string, since it's treated as a dependency.

  • 3.1.1: When restating the requirements, you should also acknowledge the discount functionality. This requirement adds to the complexity of the architecture, and helps explain some of the design decisions taken later on in the application. (That said, it relates more to the Domain Logic library than the User Interface library, so it may make more sense to restate in the introduction to Chapter 3.1.2.)

  • 3.1.1: To better articulate the distinction between the application's Product entity, DiscountedProduct DTO, and the view models, consider renaming FeaturedProductsViewModel and ProductViewModel to DiscountedProductsViewModel and DiscountedProductViewModel respectively; this makes it immediately clear that they're dealing with the DiscountedProduct DTO, and not the Product entity.

  • 3.1.1: In annotation #6 for Listing 3.4, it may well be that you "changed the ProductViewModel to accept a DiscountedProduct”, but it's not entirely clear from the code. Instead of product and featuredProducts, consider using discountedProduct and discountedProducts respectively, to really reinforce that these are instances of DiscountedProduct.

  • 3.1.1: As in Chapter 2, I'm not convinced that the progress tracking (e.g., Figure 3.6 and the paragraph before it) are necessary since three-tier architecture is so well-established and easy to follow. I think this makes more sense when there are a lot more steps involved, and especially steps that readers may not be as familiar with.

  • 3.1.2: In the first paragraph, consider adding e.g., "—i.e., they will provide the contract through which the Domain Model will interact with the forthcoming Data Access library" to further clarify exactly what those abstractions are for.

  • 3.1.2: After Listing 3.5, to really reinforce the point of the IProductService, consider adding e.g., "The IProductService represents the heart of our current Domain library in that it will bridge the User Interface library with the Data Access library. It is the glue that will bind our initial application together."

  • 3.1.2: After Listing 3.6, there's a good segment on interfaces. Because it's occurring right after the introduction of the DiscountedProduct DTO, however, it feels like a bit of a non sequitur. To address that, consider introducing it with e.g., "Before continuing, we should take a quick moment to recognize the role of interfaces in this discussion."

  • 3.1.2: In the Interfaces or abstract classes? callout, consider merging the first two sentences of the last paragraph; e.g., "When writing reusable libraries, however, the subject is becoming less clear-cut due to the need to deal with backward compatibility."

  • 3.1.2: Developers new to these patterns may get confused over the distinction between the Product Service and the Product Repository. Given that, it would be valuable to explicitly contrast them upfront. For instance, after Listing 3.7, you might say, "Note that the IProductRepository is the interface to the Data Access library, and returns 'raw' entities from the persistence store. By contrast, the IProductService will apply any business logic—such as the discount, in this case—and convert the entities to a narrower-focused DTO."

  • 3.1.2: After Listing 3.7, consider adding e.g., "It's easier to add functionality to code than it is to remove it later, once in production. This is especially true for reusable libraries which may be consumed by multiple teams and even external developers." This reinforces why it's harder to change functionality later (though, admittedly, most experienced developers should be well aware).

  • 3.1.2: In the Entity vs. DTO callout, consider adding e.g., "The Domain Object Product is an Entity because… It is also an entity because it includes behavior—in this case, the GetDiscountFor() method." This is inferred in the next paragraph, but it's such a critical distinction with DTOs that I think it's worth emphasizing.

  • 3.1.2: In the Entity vs. DTO callout, consider adding something along the lines of, "When exposing a Domain Model to external systems, we often do so with services and DTOs because we can never be sure the other system can share our type system (it may not even use .NET). An especially common example of this is when exposing entities via web services, where they must first be serialized into data-only structures such as JSON or XML, and will then be consumed by a variety of clients." Again, this just helps reinforce the point with a more concrete (and widely familiar) example.

  • 3.1.2: In the second paragraph after the Entity vs. DTO callout, consider adding e.g., "The ProductService class corresponds to Mary's class of the same name, but is now a pure Domain Model class since it doesn't have a hard-coded reference to the Data Access library (i.e., via the call to dbContext.Products)." This helps remind the reader of why Mary's ProductService wasn't a "pure Domain Model" without having to refer back to her code in Chapter 2.

  • 3.1.2: This would require a bit of refactoring, but I wonder if it wouldn't make more sense to move Listing 3.9 earlier in the chapter, before defining its dependencies. Negatively, this would mean referencing a lot of dependencies that aren't yet defined (even as interfaces). Positively, though, it would provide a clear mapping of what elements are needed, and how they fit together—much like Figure 3.3 and Figure 3.4, but perhaps less abstract. This is similar, for instance, to how IUserContext is defined after It is referenced in Listing 3.9. I think this would be especially useful for readers that may not have much experience with these patterns, and thus won’t have an intuitive read of the object names.

  • 3.1.2: In Listing 3.9, I'd recommend renaming GetFeaturedProducts() to GetDiscountedProducts(). This has two benefits. First, it helps distinguish the interface from IProductRepository; this isn't a pass-through. Second, it reinforces that this is returning instances of DiscountedProduct DTOs, and not Product entities.

  • 3.1.2: Before the second Note, consider adding e.g., "…without the HomeController having to worry about this. In other words, the HomeController doesn't need to know which role(s) are authorized for a discount price, nor is it easy for the HomeController to inadvertently enable the discount by passing e.g. a true instead of false. All the parent contexts know is that the ProductService requires an IUserContext to fulfill its responsibilities." This is really just reiterating what you've already said, but it's an important point and worth illustrating in concrete terms.

  • 3.1.2: Before the second Note, consider ending with "This reduces the overall complexity of the User Interface library" instead of "system" as the IUserContext really just moves the complexity from the User Interface library to the Domain library (where it belongs). The overall complexity of the system isn't changed that much.

  • 3.1.2: In the Dependency Inversion Principle callout, consider adding, "Both the IUserContext and IProductRepository are defined this way, even though the implementations are the responsibility of the Data Access library" just to be explicit.

  • 3.1.2: In the Dependency Inversion Principle callout, I'd consider adding e.g., "This also ensures that novel implementations can be as narrow as needed; if a different application environment needs to create its own principle, it wouldn't need to satisfy all of the requirements of IPrinciple, but only the interface points required by IUserContext."

  • 3.1.3: In the first paragraph, consider adding, "The main difference is that the CommerceContext is now only an implementation detail to our Data Access Layer, as opposed to the entirety of our data access layer. In this model, nothing outside of the Data Access Layer will have any awareness of, or dependency on, Entity Framework; it can be swapped out without any upstream effects." This is just reinforcing the point with more detail.

  • 3.1.3: Consider adding to the first Note, "In that case, we'd need the Data Access library to convert those internal persistence objects into Domain objects" to further clarify what this process would look like, and to better differentiate between "internal persistence objects" and "domain objects".

  • 3.1.4: The second-to-last paragraph should end with, "…causing it to invoke the GetFeaturedProducts() method on the ProductService instance, which in turn calls the GetFeaturedProducts() method on the SqlProductRepository instance." As is, it's skipping a step, since the HomeController doesn't directly call the SqlProductRepository.

  • 3.2.2: In the About DI Containers callout, I'd consider extending the Definition to include, "A DI Container is a library that provides DI functionality, typically by automating the construction of an object graph based on preconfigured mappings between abstractions and concrete types." Without this, the definition is a bit too vague. You do say something similar in the previous paragraph; as an alternative, you might merge that into the definition.

  • 3.2.2. In the About DI Containers, consider adding to the Warning e.g., "Critically, using a DI Container neither guarantees nor necessitates the correct usage of DI." to really emphasize that DI Containers can be used without correct implementation of DI.

  • 3.3 In the seventh bullet, I'm not entirely clear on what "without consuming classes to change" means here.


  • General Feedback
    There's no action necessary on the following; they're just comments that popped up as I was going through the book.

  • 3.1.1: In the annotations for Listing 3.3, I appreciated the turn-of-phrase "eminently susceptible" with regards to unit testing.

  • 3.1.1: As someone that has done quite a bit of internationalization and localization work, I really appreciate your Note regarding why the "ProductViewModel states the culture explicitly". I see this precise mistake quite frequently, and especially by people who are not localizing their product—which means they may never think to test their product using a different locale, and thus never discover the bug.

  • 3.1.1: I really love the emphasis on being able to "show the stakeholders a running stub of the application to comment on." So often, I find stakeholders don't take the requirements or the user interface entirely seriously until it starts to "feel real"—and if you started from the bottom up, that means inevitable changes to the requirements can be more time-consuming to implement (e.g., if they wanted to change the data model or introduce new features).

  • 3.1.1: With regard to, "Doesn't it push the burden of controlling the Dependency onto some other class?"—I had the exact same confusion when I was first introduced to Dependency Injection, and love that you're foreseeing and acknowledging that confusion so that you can address it head on. This is really helpful.

  • 3.1.2: I got a bit tripped up in this section because a lot of new pieces are being introduced without the full context of where they fit. This is somewhat necessary as you're introducing the abstractions that will later be needed to support the Data Access library without yet covering their implementations. That said, some of this may be avoidable by introducing implementations (such as the ProductService) before drilling down into dependencies (such as the IProductService, DiscountedProduct, Product, &c.). Most of my Content Suggestions for this section are about clarifying these relationships.

  • 3.1.2: In the Interfaces or abstract classes? callout, some of the distinctions may become a bit blurred in C# if they move forward with the default interface methods feature proposal.

  • 3.1.2: In the Interfaces or abstract classes? callout, I really appreciate you getting into the nuance of the design tradeoffs between interfaces and abstract classes. I especially liked the last paragraph, where you acknowledge the reasoning behind Microsoft's guidance, despite your own (very reasonable) preferences.

  • 3.1.2: In the second Note, "information that the user shouldn't be allowed to influence directly" is a really useful heuristic for determining at what level the abstraction should be introduced.


  • Jeremy
    Steven van Deursen (32) [Avatar] Offline
    #2
    Hi Jeremy,


    Thanks again. Your feedback is the gift that keeps on giving smilie

    I just finished processing your comments and incorporated most of your feedback.

    There are two comments that I would like to respond to explicitly.

    In Figure 3.3, UserContextAdapter, should be under the Data Access library


    The UserContextAdapter depends on ASP.NET Core's HttpContextAccessor. Placing it in the Data Access library would therefore make the DAL dependent on ASP.NET Core, which is typically not desirable when we want to run the DL and DAL in a different context (e.g. a Windows service). I'm still thinking about describing that decision explicitly in the chapter.

    the distinctions may become a bit blurred in C# if they move forward with the default interface methods feature proposal.

    Yes, that might indeed be the case. I must say that I'm not fond of this C# proposal _at all_.


    Cheers
    Jeremy Caney (28) [Avatar] Offline
    #3
    Steven—

    Oh, sheesh—you're (obviously) right about the AspNetUserContextAdapter! I do apologize.

    I actually think you describe the decision over where to place it really well in 3.1.4 Composing the application in the Composition Root.

    Ultimately, I suspect that I was tripping over the overlap, in this case, between the Composition Root and the User Interface library—which you explore more in 4.1.1 How Composition Root works.

    (Now, how that translated to "[it] should be under the Data Access library" in my feedback is a mystery I don't have a good answer for!)

    Jeremy