This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

A while ago I said:

Seriously?!  22(!) projects to do a sample application using Northwind?

And people took me up to task about it. The criticism was mostly focused on two parts:

• I didn’t get that the project wasn’t about Northwind, but about being a sample app for architectural design patterns.
• I couldn’t actually decide that a project was bad simply by looking at the project structure and some minor code browsing.

I am sad to say that after taking a detailed look at the code, I am even more firmly back at my original conclusion.  I started to do a review of the UI code, but there really is no real need to do so.

The entire project, as I said in the beginning, is supposed to be a sample application for Northwind. Northwind is a CRUD application. Well, not exactly, it is supposed to be an example of an Online Store, which is something much bigger than just Northwind. But it isn’t.

Say what you will, the Northwind Starter Kit is a CRUD application. It does exactly that, and nothing else. It does so in an incredibly complicated fashion, mind, but that is what it does.

Well, it doesn’t do updates, or deletes, or creates. So it is just an R application (I certainly consider the codebase to be R rated, not for impressionable developers).

If you want to have a sample application to show off architectural ideas, make sure that the application can actually, you know, show them. The only thing that NSK does is loading stuff from the database, try as I might, I found no real piece of business logic, no any reason why it is so complicated.

So, to the guys who commented on that, it isn’t a good project. If you like it, I am happy for you, there are also people who loves this guy:

Personally, I would call pest control.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

It is obvious from reading the code that there was some attention given to CQRS. Unfortunately, I can’t really figure out what for.

To start with, both the Read Model and the Domain Model are actually sitting on the same physical location. If you are doing that, there is a 95% chance that you don’t need CQRS. If you have that, you are going to waste a lot of time and effort and are very unlikely to get anything from it.

In the case of NSK, here is the domain model vs. the read model for the customer.

I marked the difference.

I am sorry, there is nothing that justify a different model here. Just needless complexity.

Remember, our job is to make things simpler, not make it hard to work with the application.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

Okay, enough about the data access parts. Let us see take a look at a few of the other things that are going on in the application. In particular, this is supposed to be an application with…

Domain logic is implemented by means of a Domain Model, onto a layer of services adds application logic. The model is persisted by a DAL designed around the principles of the "Repository" patterns, which has been implemented in a LINQ-friendly way.

Let us try to figure this out one at a time, okay?

The only method in the domain model that have even a hint of domain logic is the CalculateTotalIncome method. Yes, you got it right, that is a method, as in singular. And that method should be replaced with a query, it has no business being on the domain model.

So let us move to the services, okay? Here are the service definitions in the entire project:

Look at the methods carefully. Do you see any action at all? You don’t, the entire thing is just about queries.

And queries should be simple, not abstracted away and made very hard to figure out.

The rule of the thumb is that you try hard to not abstract queries, it is operations that you try to abstract. Operations is where you usually actually find the business logic.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

One of the things that I repeatedly call out is the forwarding type of architecture, a simple operation that is hidden away by a large number of abstractions that serves no real purpose.

Instead of a controller, let us look at a web service, just to make things slightly different. We have the following:

Okay, let us dig deeper:

I really like the fact that this repository actually have have FindById method, which this service promptly ignores in favor of using the IQueryable<Customer> implementation. If you want to know how that is implemented, just look (using the EF Code First repository implementations, the others are fairly similar):

 

All in all, the entire thing only serves to make things harder to understand and maintain.

Does anyone really think that this abstraction adds anything? What is the point?!

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

The database is usually a pretty important piece in your application, and it likes to remind you that it should be respected. If you don’t take care of that, it will make sure that there will be a lot of pain in your future. Case in point, let us look at this method:

It looks nice, it is certainly something that looks like a business service. So let us dig down and see how it works.

It seems like a nice thing, the code is clear, and beside the bug where you get 100% discount if you buy enough and the dissonance between the comment and the code, fairly clear. And it seems that we have service logic and entity logic, which is always nice.

Except that this piece of code issues the following queries (let us assume a customer with 50 orders).

1 Query to load the customer, line 34 in this code. And now let us look at line 35… what is actually going on here:

Okay, so we have an additional query for loading the customer’s orders. Let us dig deeper.

And for each order, we have another query for loading all of that order’s items. Does it gets worse?

Phew! I was worried here for a second.

But it turns out that we only have a Select N+2 here, where N is the number of orders that a customer has.

What do you want, calculating the discount for the order is complicated, it is supposed to take a lot of time. Of course, the entire thing can be expressed in SQL as:

SELECT 
  SUM((UnitPrice * Quantity) * (1 - Discount) Income
FROM OrderItems o
WHERE o.OrderID in (
  SELECT Id FROM Orders
  WHERE CustomerId = @CustomerId
)

But go ahead and try putting that optimization in. The architecture for the application will actively fight you on that.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

In my previous post, I talked about how the CatalogController.Product(id) action was completely ridiculous in the level of abstraction that it used to do its work and promised to show how to do the same work on an actual read model in a much simpler fashion. Here is the code.

There are several things that you might note about this code:

• It is all inline, so it is possible to analyze, optimize and figure out what the hell is going on easily.
• It doesn’t got to the database to load data that it already has.
• The code actually does something meaningful.
• It only do one thing, and it does this elegantly.

This is actually using a read model. By, you know, reading from it, instead of abstracting it.

But there is a problem here, I hear you shout (already reaching for the pitchfork, at least let me finish this post).

Previously, we have hidden the logic of discontinued products and available products behind the following abstraction:

Now we are embedding this inside the query itself, what happens if this logic changes? We would now need to go everywhere we used this logic and update it.

Well, yes. Except that there are two mitigating factors.

• This nice abstraction is never used elsewhere.
• It is easy to create our own abstraction.

Here is an example on how to do this without adding additional layers of abstractions.

Which means that our action method now looks like this:

Simple, easy, performant, maintainable.

And it doesn’t make my head hurt or cause me to stay up until 4 AM feeling like an XKCD item.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

In my last few posts, I have gone over the data access strategy used in NSK. I haven’t been impressed. In fact, I am somewhere between horrified, shocked and amused in the same way you feel when you see a clown slipping on a banana peel.  Why do I say that? Let us trace a single call from the front end all the way to the back.

The case in point, CatalogController.Product(id) action. This is something that should just display the product on the screen, so it should be fairly simple, right? Here is how it works when drawn as UML:

To simplify things, I decided to skip any method calls on the same objects (there are more than a few).

Let me show you how this looks like in actual code:

Digging deeper, we get:

We will deal with the first method call to CatalogServices now, which looks like:

I’ll skip going deeper, because this is just a layer of needless abstraction on top of Entity Framework and that is quite enough already.

Now let us deal with the second call to CatalogServices, which is actually more interesting:

Note the marked line? This is generating a query. This is interesting, because we have already loaded the product. There is no way of optimizing that, of course, because the architecture doesn’t let you.

Now, you need all of this just to show a single product on the screen. I mean, seriously.

You might have noticed some references to things like Read Model in the code. Which I find highly ironic. Read Models are about making the read side of things simple, not drowning the code in abstraction on top of abstraction on top of abstraction.

In my next post, I’ll show a better way to handle this scenario. A way that is actually simpler and make use an of actual read model and not infinite levels of indirection.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

In my previous posts, I focused mostly on the needlessness of the repositories implementation and why you want to avoid that (especially implementing it multiple times). In this post, I want to talk about other problems regarding the data access. In this case, the sudden urge to wash my hands that occurred when I saw this:

I mean, you are already using an OR/M. I don’t like the Repository implementation, but dropping down to SQL (and unparapetrized one at that) seems uncalled for.

By the way, the most logical reason for this to be done is to avoid mapping the Picture column to the category, since the OR/M in use here (EF) doesn’t support the notion of lazy properties.

Again, this is a problem when you are trying to use multiple OR/Ms, and that is neither required nor really useful.

Okay, enough about the low level data access details. On my next post I’ll deal with how those repositories are actually being used.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

Update: Andrea, the project’s leader has posted a reply to this series of posts.

Yes, this is another repositories are evil if you are using an OR/M post.

That is probably going to cause some reaction, so I am going to back this up with code from this NSK project. Let us talk about repositories, in particular. Let us see what we have here:

Okaay…

Now here are a few problems that I have with this:

• There is no value gained by introducing this abstraction. You aren’t adding any capability what so ever.
• In fact, since all OR/Ms provide an abstraction that isn’t dependent on type, creating IRepository<T> and things like ICustomerRepository is just making things more complicated.
• There are going to be changes in behavior between different repositories implementations that will break your code.

Let us see what we actually have as a result. This is the Entity Framework POCO implementation:

You can probably guess how the rest of it is actually implemented. Yes, we have a LOT of code that is dedicated solely for this sort of forwarding operations.

And then we have the actual implementation of the delete:

Just to remind you, here is the NHibernate implementation of the same function:

Leaving aside the atrocious error handling code, the EF POCO version will do an immediate delete. The NHibernate version will wait for the transaction to be committed.

And don’t worry, I do remember the error handling. This is simply wrong.

And then we have implementations such as this:

This is for the Entity Framework Code First implementation. There is a message here that is coming to me loud and clear. This code wants to be deleted. It is neglected and abused and doesn’t serve any purpose in life except gobble up pieces of valuable disk space that could be filled with the much more valuable result of  reading from/dev/random.

This is a review of the Northwind Starter Kit project, this review revision 94815 from Dec 18 2011.

Update: Andrea, the project’s leader has posted a reply to this series of posts.

I like to start reviewing applications from their database interactions. That it usually low level enough to tell me what is actually going on, and it is critical to the app, so a lot of thought usually goes there.

In good applications, I have hard time finding the data access code, because it isn’t there. It is in the OR/M or the server client API (in the case of RavenDB). In some applications, if they work against legacy databases or without the benefit of OR/M or against a strange data source (such as a remote web service target) may need an explicit data layer, but most don’t.

NSK actually have 5 projects dedicated solely to data access. I find this.. scary.

Okay, let me start outlying things in simple terms. You don’t want to do things with regards to data access the way NSK does them.

Let us explore all the ways it is broken. First, in terms of actual ROI. There is absolutely no reason to have multiple implementations with different OR/Ms. There is really not a shred of reason to do so. The OR/M is already doing the work of handling the abstraction from the database layer, and the only thing that you get is an inconsistent API, inability to actually important features and a whole lot more work that doesn’t' get you anything.

Second, there are the abstractions used:

I don’t like repositories, because they abstract important details about the way you work with the database. But let us give this the benefit of the doubt and look at the implementation. There is only one implementation of IRepository, which uses NHibernate.

As you can see, this is pretty basic stuff. You can also see that there are several methods that aren’t implemented. That is because they make no sense to a data. The reason they are there is because IRepository<T> inherits from ICollection<T>. And the reason for that is likely because of this:

Mediates between the domain and data mapping layers using a collection-like interface for accessing domain objects.

The fact that this is totally the wrong abstraction to use doesn’t enter to the design, it seems.

Note that we also violate the contract of ICollection<T>.Remove:

true if item was successfully removed from the ICollection<T>; otherwise, false. This method also returns false if item is not found in the original ICollection<T>.

There are other reasons to dislike this sort of thing, but I’ll touch on that on my next post.