Northwind Starter Kit ReviewFrom start to finishing–tracing a request
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?!
More posts in "Northwind Starter Kit Review" series:
- (26 Jan 2012) Conclusion
- (24 Jan 2012) That CQRS thing
- (23 Jan 2012) It is all about the services
- (20 Jan 2012) From start to finishing–tracing a request
- (18 Jan 2012) If you won’t respect the database, there will be pain
- (16 Jan 2012) Refactoring to an actual read model
- (13 Jan 2012) Data Access review thoughts
- (12 Jan 2012) The parents have eaten sour grapes, and the children’s teeth are set on edge
- (11 Jan 2012) Data Access and the essence of needless work, Part II
- (10 Jan 2012) Data Access and the essence of needless work, Part I
Comments
The point is to increase the sales of your ORM profiler.
IMO, the inherent failure here is the dependency this author has taken: ICustomerRepository. ICustomerRepository then implements a generic repository through inheritance which is epic fail. Generic repositories only provide value if they're produced through composition, not inheritance.
If this code showed:
public CustomerDetailWindowWorkerServices(IRepository<Customer> customerRepository)
it would likely have far more value.
Completing repository implementations through inheritance is a gigantic anemic model problem. Leaving you with tons of useless classes like:
public CustomerRepository : GenericRepository<Customer> { Public Customer FindById(string id) { return base.FindById(id) } }
Or other noise abstractions. Or even entirely empty classes
public CustomerRepository : GenericRepository<Customer> { }
^ can't get any more anemic than that.
IMO, the inherent failure here is the dependency this author has taken: ICustomerRepository. ICustomerRepository then implements a generic repository through inheritance which is epic fail. Generic repositories only provide value if they're produced through composition, not inheritance.
If this code showed:
public CustomerDetailWindowWorkerServices(IRepository<Customer> customerRepository)
it would likely have far more value.
Completing repository implementations through inheritance is a gigantic anemic model problem. Leaving you with tons of useless classes like:
public CustomerRepository : GenericRepository<Customer> { Public Customer FindById(string id) { return base.FindById(id) } }
Or other noise abstractions. Or even entirely empty classes
public CustomerRepository : GenericRepository<Customer> { }
^ can't get any more anemic than that.
First post had all my generics butchered, hopefully this one works.
@dotnetchris
I like your assertion that "Generic repositories only provide value if they're produced through composition, not inheritance." I've seen this done many times before and never been comfortable with it.
Would you propose that, instead of
CustomerRepository : GenericRepository<Customer>{ }
Use something like:
CustomerRepository : ICustomerRepository { GenericRepository r {get;set;}
}
Or am I missing your point?
Despite many worthy views to the contrary I stick to lines of code at my favourite metric. It encourages the team to stop writing rubbish that others have to decipher and maintain, and to cut out layers of abstraction when they find them.
When your model is just a bag of properties and your application is just a CRUD you realy dont need repository. When you have complex model it is another story. It is a common case for commands when I should access one aggregate from other and than I prefer to inject repository into my entities instead of ISession. Just dont like to see NH use in a model. For queries I dont fetch model entities and than convert them to DTOs. I map DTOs to DB and directly use ISession in my query handlers.
@Stan
You are not supposed to have data access code (whether it is repositories or ADO .NET classes) inside entities. That is the whole point of the POCO concept.
So I'm guessing your problem is with the Repository and not the Service?
Because in my opinion that service performs a clear purpose, it constructs a view Customer. Admittedly it seems as though the service and view could have been named better. I am also more than happy to see code like this NOT happen in the body of a controller action for all but the most simplest of scenarios.
constructs a view of a Customer*
+1 to Dmitry. Having data access in your domain model is an absolute nightmare. Even though you can mock your DA interfaces it makes for way uglier tests.
Comment preview