The wages of sinRe-creating the Stored Procedure API in C#
Originally posted at 3/10/2011
This time, this is a review of the Sharp Commerce application. Again, I have stumbled upon the application by pure chance, and I have very little notion about who wrote it. The problem is that this system seems to be drastically more complicated than it should be.
In this case, I want to look at the type of API that is exposed:
If this reminds you of the bad old days of having only Stored Procedure API available, that is not by chance. Far worst than that, however, is the call paths where this is used.
- IEmailTemplateRepository.Get(EmailTemplateLookup) is implemented as
public EmailTemplate Get(EmailTemplateLookup emailId) { return Get((int)emailId); }
and is only used in:- EmailService.Get(EmailTemplateLookup) whose implementation is:
public EmailTemplate GetEmail(EmailTemplateLookup template) { return emailTemplateRepository.Get(template); }
- EmailService.Get(EmailTemplateLookup) whose implementation is:
- ICategoryRepository.GetParentCategories is only used from:
- CategoryService.GetParentCategories which is implemented as:
public IEnumerable<Category> GetParentCategories() { IEnumerable<Category> categories = categoryRepository.GetParentCategories(); return categories; }
- CategoryService.GetParentCategories which is implemented as:
- CurrencyService.GetEnabledCurrencies which is implemented as:
public IEnumerable<Currency> GetEnabledCurrencies() { return currencyRepository.GetEnabledCurrencies(); }
For that matter, let us take a look at the entire CurrencyService class, shall we?
public class CategoryService : ICategoryService { private readonly ICategoryRepository categoryRepository; public CategoryService(ICategoryRepository categoryRepository) { this.categoryRepository = categoryRepository; } public IList<Category> GetCategories() { return categoryRepository.GetAll(); } public Category GetCategory(int id) { return categoryRepository.Get(id); } public void SaveOrUpdate(Category categoryModel) { categoryRepository.SaveOrUpdate(categoryModel); } public void Delete(Category category) { categoryRepository.Delete(category); } public IEnumerable<Category> GetParentCategories() { IEnumerable<Category> categories = categoryRepository.GetParentCategories(); return categories; } }
To be honest, I really don’t see the point.
Now, just a hint on the next few posts, there are places where I think wrapping the usage of the NHibernate API was a good idea, even if I strongly disagree with how this was done.
More posts in "The wages of sin" series:
- (24 Mar 2011) Hit that database one more time…
- (23 Mar 2011) Inverse leaky abstractions
- (22 Mar 2011) Proper and improper usage of abstracting an OR/M
- (21 Mar 2011) Re-creating the Stored Procedure API in C#
- (18 Mar 2011) Over architecture in the real world
Comments
I wrote the same in russian. And the main idea of my post is to combine query extensions with each other. So you can have query extension for Active field checking, or for checking permissons. And if you have same UserControl for show Customers in many pages, but with different count of items, or with different UI you can get query extensions for getting customers and then add some conditions like .Take() or Fetch() in UI layer, because UI layer know what exactly you want to show. Repositories is overhead. But I think it's old school, and people who used to write code with repositories will continue do it.
These kinds of repositories are very handy when you're data sources aren't limited to relational databases.
With this kind of architecture you can keep the email messagesin a relational database, while you store the templates in a document database like RavenDB.
And suddenly the abstraction of NHibernate code doesn't look that bad anymore with the side node that the initial implementation also uses different data sources to justify the added overhead..
@Ayende
In this example you showed us, the service class is very simple and doesn't contain any business logic. I suppose other service classes in the project contains more code than that, like validation and exception handling. But because a service layer was adopted to wrap the DAL for every Domain objects, the developer had to stick with it thoughout the system to make the architecture consistent.
I also use service classes to wrap my DAL to make them reusable in my controller/presenter classes and also in another communication layer (WCF and the like).
I think you are confusing the fact that this is not wrapping the usage of the NHibernate API but rather the DAL itself.
Hi Oren,
I was very surprised today to find my project featured on your site! I'm actually quite chuffed.
Firstly I would just like to mention that this project was something I was working on last year mainly to play around with Sharp Architecture and NHibernate and was never really meant to be released and should be taken as a prototype code.
Regarding all the abstractions in the project to be honest I have to partially agree with you on that one I did try hard to abstract everything but that was partly because I was experimenting with Sharp Architecture and trying to follow their architecture design and best practices.
I think abstractions are generally good and have worked in a few projects where the code is just too tightly coupled and extremely hard to maintain as you end up breaking 10 things with 1 change. Obviously it's all about coming up with the right balance of it and knowing when you need it or not.
I personally think having a data access layer separate from your application services is a good thing although in some of my projects I just work with the Entity Framework Context in my controllers.
Thanks for featuring my project :)
"I personally think having a data access layer separate from your application services is a good thing "
What do you mean by this? That's the whole point of ORM, your data access is abstracted away from low-level nastiness and instead you have a nice API.
I think that's what people don't get. They view ORM like we're still doing low-level ADO calls. NHibernate IS your abstraction. There's really no need to abstract it further unless you have a very good reason.
@Dave,
The problem is this: the amount of work you saved by implementing this repository pattern up front is going to be equal to or greater than the work required to just put RavenDB calls in where they're needed after the fact. (Unless you have mapped relations to/from whichever table you're trying to move to Raven, in which case you'll have a ton of work regardless of whether you use the repositories)
S#arp Architecture is mentioned in http://nhforge.org...
This is an image on a code rant you did in a blog post about the asp.net mvc music store on 5/13/2010.
ayende.com/.../image_thumb_1.png
You complained about have the dbset exposed to the controller...
Whats the difference here?
@nick, the whole point of an ORM is to map objects to relational data, plain and simple. Whether or not there is an abstraction there like a repository is orthogonal to ORM. Why are we trying to redefine what an ORM is? Why not focus the argument on whether or not there should be an abstraction between your domain and persistence library?
I personally try to avoid repository classes.
They make an application harder to maintain, especially when you're writing classes that depend on several repository classes, when you could just inject a single instance of ISession.
They make an application slower/inflexible - as you lose all the advanced features of NHibernate; such as futures.
The relational data source is abstracted already (as long as you're gonna stick with relational databases). Besides most development teams do not switch to another persistance technology halfway through a project.
Plus, I think it'd be easy to switch to another data source if you were already using NHibernate directly. You'll end up spending just as much time on switching over, as you would've done developing your repositories and suffering with the friction they cause.
@Simon What specifically about using a repository precludes you from using futures? Just return an IEnumerable.
I'm personally at an awkward stage with repositories. I don't particularly like them, but there are aspects of them which are useful. The argument of losing the features of NHibernate is a valid one, but simply exposing an ISession is asking for duplication explosion; it's arguably no better than exposing a SqlConnection and letting all the services have at it. If you have two areas of a system that require the same query, you should be encapsulating and reusing it. Where do you put the shared queries? In a repository.
I'd be happy to ditch the repository entirely if it didn't mean duplication of queries and potential future maintenance nightmare (change every query where X joins Y).
@James Gregory: It depends on what the query is. Doing ISession.Get <mytype( id ) all over the place doesn't seem so bad.
If the queries get larger, I really like the idea that Ayende mentioned in his ayende.com/.../...it-of-doom-the-evils-of-the.aspx> post about using extension methods to do query filters seems to work out really nicely. It allow for code to be put in a single place and allows for you to still use the session. You are able to compose your query with several filters too, which might even make the query more readable.
@Kelly B,
the example to which you link is doing writes. To oversimplify and paraphrase Ayende, he prefers abstracting away writes, but not reads.
ayende.com/.../...-simplicity-as-a-core-value.aspx
Having too much abstraction has definitely been a problem in my own code. It got worst when I started doing unit testing, its so easy to mock out other layers why not add some more. After some experience with that I came to see how I was doing it wrong. A couple people have said the repository is necessary for testing which leads me to think others are making the same mistake.
Now I try to follow the practices as described in Freeman + Pryce's "Guided by Tests". Here testing doesn't start with unit testing, but a spanning test that runs the full system without mocks. What I've found when I do this is that my software needs fewer internal layers of abstraction. I only add those layers as they are actually needed according to the tests, preventing architecture overkill. The tests are harder to write, and slow, but the tests do a better job of verifying the system and they evolve with the code more easily. Unit tests come in later when trying to get weird edge cases that are too difficult to reach with existing spanning tests, but only after there are spanning tests hitting the tested components verifying they work together.
It was as I reviewed RavenDB's test code that I realized I had been doing it wrong. RavenDB's tests don't mock internal components, or isolate layers with every test. This forced me to rethink my test strategy a bit.
I don't think the unit-test-first-with-lots-of-layers approach is total folly, I have some projects where thats what I did and things worked out fine. The tests still provide a safety net, but since they capture internal abstractions its harder to change implementation without having a test maintenance cost.
@James true, now NH3 does supports futures on IEnumerable :)
I think if you're using strongly-typed queries (QueryOver or LINQ) then maintenance isn't such a big deal when you have good tooling. If however you still have some raw SQL or queries reliant on strings, then yes perhaps these should be grouped in a query object.
@Simon: NH has done futures on IEnumerable since (at least) 2.1.2, which is what we're running at my day job.
I don't think your choice of query mechanism makes much difference. I don't have an issue with simple queries (select x where x.y = z), it's the more complex ones I'm concerned about repeating. Aggregates, group bys, projections, etc... We have quite a few complex queries that are used throughout the system that I would not want reimplementing every time they're used. Of course these should be abstracted, but apart from a repository I don't really see any other alternative. Restrained use of query objects is probably the way to go, thinking about it.
@Josh: Extension methods are nice, but they're really nothing more than syntactic sugar over shoving them into static helper classes.
@James Yeah, I see your point now :) I personally would use query objects rather than repositories for these.
I didn't realise NH had futures on IEnumerable before 3.0, I remember Ayende had to do some work on it a few months ago for the new OOTB LINQ provider. Plus I haven't used NH pre-3.0 a great deal.
@Nick Aceves. I don't know where the data is coming from. I have written a framework and the use of that framework dictates where the data is stored.
Repositories provide an abstraction layer between the business code and the actual CRUD methods. If you limit yourself to NHibernate, you limit your code to relational databases.
Because we use repositories data can be stored on a web service, WCF service, document database, XML document, CSV files, MS Exchange contacts, LDAP, etc, etc.
Like I mentioned in my side node, such a repository abstraction can only be justified in a generic framework or when you from the get-go use multiple data sources.
If you have an app where you exactly know where the data comes from, this may all be true. But as Dave said, if the data comes from different sources there is no other way than using an abstraction to the DAL. We use repositories exactly for that reason. So no one has to think about where the customer entities are coming from...directly form the DB or via WCF?
Another point are queries that are used all over the place. To have single methods for them is easier to handle than writing the query code over and over again. But this does'nt meen to have an repository for each and every thing. So i think the CategoryService from the example above can live good without the use of an extra repository class, if you know that NH is the only data source for it.
Comment preview