I was reading this stack overflow question: How can I solve this: Nhibernate Querying in an n-tier architecture?
The author is trying to abstract away NHibernate and is being counseled rather heavily not to do so. In the comments there are a couple of blog entries by Ayende on this topic:
The false myth of encapsulating data access in the DAL
Architecting in the pit of doom the evils of the repository abstraction layer
Ayende is pretty down on abstracting away NHIbernate. The answers on StackOverflow push the questioner toward just standing up an in-memory Sqlite instance and executing the tests against that.
The Sqlite solution is pretty painful with complex databases. It requires that you set up an enormous amount of data that isn’t really germane to your test in order to satisfy FK and other constraints. The ceremony of creating this extra data clutters the test and obscures the intent. To test a query for employees who are managers, I’d have to create Departments and Job Titles and Salary Types etc., etc., etc.. Dis-like.
What problem am I trying to solve?
In the .NET space developers tend to want to use LINQ to access, filter, and project data. NHibernate (partially) supports LINQ via an extension method off of ISession. Because ISession.Query<T> is an extension method, it is not stubbable with free mocking tools such as RhinoMocks, Moq, or my favorite: NSubstitute. This is why people push you to use the Sqlite solution—because the piece of the underlying interface that you want to use most of the time is not built for stubbing.
I think that a fundamental problem with NHibernate is that it is trying to serve 2 masters. On the one hand it wants to be a faithful port of Hibernate. On the other, it wants to be a good citizen for .NET. Since .NET has LINQ and Java doesn’t, the support for LINQ is shoddy and doesn’t really fit in well the rest of the API design. LINQ support is an “add-on” to the Java api, and not a first-class citizen. I think this is why it was implemented as an extension method instead of as part of the ISession interface.
I firmly disagree with Ayende on Generic Repository. However, I do agree with some of the criticisms he offers against specific implementations. I think his arguments are a little bit of straw man, however. It is possible to do Generic Repository well.
I prefer to keep my IRepository interface simple:
public interface IRepository : IDisposable { IQueryable<T> Find<T>() where T: class; T Get<T>(object key) where T : class; void Save<T>(T value) where T: class; void Delete<T>(T value) where T: class; ITransaction BeginTransaction(); IDbConnection GetUnderlyingConnection(); }
Here are some of my guidelines when using a Generic Repository abstraction:
- My purpose in using Generic Repository is not to “hide” the ORM, but
- to ease testability.
- to provide a common interface for accessing multiple types of databases (e.g., I have implemented IRepository against relational and non-relational databases) Most of my storage operations follow the Retrieve-Modify-Persist pattern, so Find<T>, Get<T>, and Save<T> support almost everything I need.
- I don’t expose my data models outside of self-contained operations, so Attach/Detach are not useful to me.
- If I need any of the other advanced ORM features, I’ll use the ORM directly and write an appropriate integration test for that functionality.
- I don’t use Attach/Detach, bulk operations, Flush, Futures, or any other advanced features of the ORM in my IRepository interface. I prefer an interface that is clean, simple, and useful in 95% of my scenarios.
- I implemented Find<T> as an IQueryable<T>. This makes it easy to use the Specification pattern to perform arbitrary queries. I wrote a specification package that targets LINQ for this purpose.
- In production code it is usually easy enough to append where-clauses to the exposed IQueryable<T>
- For dynamic user-driven queries I will write a class that will convert a flat request contract into the where-clause needed by the operation.
- I expose the underlying connection so that if someone needs to execute a sproc or raw sql there is a convenient way of doing that.
Disclaimer: I do not work for a startup. I have never worked for a startup. I am not interested in working for a startup.
Uncle Bob recently wrote an interesting post called “The Startup Trap” which prompted Greg Young at codebetter.com to respond with “Startups and TDD”.
The heart of their disagreement can be captured in two quick quotes:
As time passes your estimates will grow. You’ll find it harder and harder to add new features. You will find more and more bugs accumulating. You’ll start to parse the bugs into critical and acceptable (as if any bug is acceptable!) You’ll create modules that are so fragile you won’t trust yourself, or anyone else, to modify them; so you’ll work around them. You’ll build a festering pile of code that, with every passing week, requires more and more effort just to keep running. Forward progress will slow and falter. It may even reverse as each release becomes buggier and buggier, and less and less stable. Catastrophes will become more and more common as errors, that should never have happened, create corruptions and damage that take huge traunches of time to repair.
–Uncle Bob
What really mattered was that after our nine months of beautiful architecture and coding work we were making approximately 10k/month more than what our stupid production prototype made for all of its shortcomings.
We would have been better off making 30 new production prototypes of different strategies and “throwing shit at the wall” to see what worked than spending any time beyond a bit of stabilization of the first. How many new business opportunities would we have found?
— Greg Young (emphasis in original)
I disgree with the advice that Mr. Young seems to be giving. My initial comment on his post was:
I agree that you shouldn’t have spent a bunch of time building a new application alongside your prototype. You did the right thing in shoring it up and fixing the worst pain points. I personally do not believe in building a green-field app when you already have a working brown-field one.
…
I’m curious, is your prototype app still in use? Did it survive?
I can understand why Mr. Young’s attitude may be tempting for some developers to embrace, but how would we feel if we heard a comment like this from a used-car salesman? Would you want to do business with a salesman that would sell you a car that was held together with duct-tape and baling wire and then spend his time looking for other business opportunities while you’re stuck using his pile of shit?
Let me ask the question another way. Is working for a startup an excuse to churn out crap software and move on to the next big idea before the company that just paid you starts to notice that festering pile of rot you just created for them?
I’m not personally accusing Mr. Young of having this attitude, but it does seem to capture the attitude I’ve heard expressed by some developers in the startup world.
Update: Uncle Bob’s follow-up.
One of the points I tried to make in my talk about TDD yesterday is that TDD is more focused on the clarity and expressiveness of your code than on its actual implementation. I wanted to take a little time and expand on what I meant.
I used a Shopping Cart as an TDD sample. In the sample, the requirement is that as products are added to the shopping cart, the cart should contain a list or OrderDetails that are distinct by product sku. Here is the test I wrote for this case (this is commit #8 if you want to follow along):
[Test] public void Details_AfterAddingSameProductTwice_ShouldDefragDetails() { // Arrange: Declare any variables or set up any conditions // required by your test. var cart = new Lib.ShoppingCart(); var product = new Product() { Sku = "ABC", Description = "Test", Price = 1.99 }; const int firstQuantity = 5; const int secondQuantity = 3; // Act: Perform the activity under test. cart.AddToCart(product, firstQuantity); cart.AddToCart(product, secondQuantity); // Assert: Verify that the activity under test had the // expected results Assert.That(cart.Details.Count, Is.EqualTo(1)); var detail = cart.Details.Single(); var expectedQuantity = firstQuantity + secondQuantity; Assert.That(detail.Quantity, Is.EqualTo(expectedQuantity)); Assert.That(detail.Product, Is.SameAs(product)); }
The naive implementation of AddToCart is currently as follows:
public void AddToCart(Product product, int quantity) { this._details.Add(new OrderDetail() { Product = product, Quantity = quantity }); }
This implementation of AddToCart fails the test case since it does not account for adding the same product sku twice. In order to get to the “Green” step, I made these changes:
public void AddToCart(Product product, int quantity) { if (this.Details.Any(detail => detail.Product.Sku == product.Sku)) { this.Details.First(detail => detail.Product.Sku == product.Sku).Quantity += quantity; } else { this._details.Add(new OrderDetail() { Product = product, Quantity = quantity }); } }
At this point, the test passes, but I think the above implementation is kind of ugly. Having the code in this kind of ugly state is still a value though because now I know I have solved the problem correctly. Let’s start by using Extract Condition on the conditional expression.
public void AddToCart(Product product, int quantity) { var detail = this.Details.SingleOrDefault(d => d.Product.Sku == product.Sku); if (detail != null) { detail.Quantity += quantity; } else { this._details.Add(new OrderDetail() { Product = product, Quantity = quantity }); } }
The algorithm being used is becoming clearer.
- Determine if I have an OrderDetail matching the Product Sku.
- If I do, increment the quantity.
- If I do not, create a new OrderDetail matching the product sku and set it’s quantity.
It’s a pretty simple algorithm. Let’s do a little more refactoring. Let’s apply Extract Method to the lambda expression.
public void AddToCart(Product product, int quantity) { var detail = GetProductDetail(product); if (detail != null) { detail.Quantity += quantity; } else { this._details.Add(new OrderDetail() { Product = product, Quantity = quantity }); } } private OrderDetail GetProductDetail(Product product) { return this.Details.SingleOrDefault(d => d.Product.Sku == product.Sku); }
This reads still more clearly. This is also where I stopped in my talk. Note that it has not been necessary to make changes to the my test case because the changes I have made go to the private implementation of the class. I’d like to go a little further now and say that if I change the algorithm I can actually make this code even clearer. What if the algorithm was changed to:
- Find or Create an OrderDetail matching the product sku.
- Update the quantity.
In the first algorithm, I am taking different action with the quantity depending on whether or not the detail exists. In the new algorithm, I’m demoting the importance of whether the order detail already exists so that I can always take the same action with respect to the quantity. Here’s the naive implementation:
public void AddToCart(Product product, int quantity) { OrderDetail detail; if (this.Details.Any(d => d.Product.Sku == product.Sku)) { detail = this.Details.Single(d => d.Product.Sku == product.Sku); } else { detail = new OrderDetail() { Product = product }; this._details.Add(detail); } detail.Quantity += quantity; }
The naive implementation is a little clearer. Let’s apply some refactoring effort and see what happens.. Let’s apply Extract Method to the entire process of getting the order detail.
public void AddToCart(Product product, int quantity) { var detail = GetDetail(product); detail.Quantity += quantity; } private OrderDetail GetDetail(Product product) { OrderDetail detail; if (this.Details.Any(d => d.Product.Sku == product.Sku)) { detail = this.Details.Single(d => d.Product.Sku == product.Sku); } else { detail = new OrderDetail() { Product = product }; this._details.Add(detail); } return detail; }
This is starting to take shape. However, “GetDetail” does not really communicate that we may be creating a new detail instead of just returning an existing one. If we rename it to FindOrCreateOrderDetailForProduct, we may get that clarity.
public void AddToCart(Product product, int quantity) { var detail = FindOrCreateDetailForProduct(product); detail.Quantity += quantity; } private OrderDetail FindOrCreateDetailForProduct(Product product) { OrderDetail detail; if (this.Details.Any(d => d.Product.Sku == product.Sku)) { detail = this.Details.Single(d => d.Product.Sku == product.Sku); } else { detail = new OrderDetail() { Product = product }; this._details.Add(detail); } return detail; }
AddToCart() looks pretty good now. It’s easy to read, and each line communicates the intent of our code clearly. FindOrCreateDetailForProduct() on the other hand is less easy to read. I’m going to apply Extract Conditional to the if statement, and Extract Method to each side of the expression. Here is the result:
private OrderDetail FindOrCreateDetailForProduct(Product product) { var detail = HasProductDetail(product) ? FindDetailForProduct(product) : CreateDetailForProduct(product); return detail; } private OrderDetail CreateDetailForProduct(Product product) { var detail = new OrderDetail() { Product = product }; this._details.Add(detail); return detail; } private OrderDetail FindDetailForProduct(Product product) { var detail = this.Details.Single(d => d.Product.Sku == product.Sku); return detail; } private bool HasProductDetail(Product product) { return this.Details.Any(d => d.Product.Sku == product.Sku); }
Now I’ve noticed that HasProductDetail and FindDetailForProduct are only using the product sku. I’m going to change the signature of these methods to accept only the sku, and I’ll change the method names accordingly.
public void AddToCart(Product product, int quantity) { var detail = FindOrCreateDetailForProduct(product); detail.Quantity += quantity; } private OrderDetail FindOrCreateDetailForProduct(Product product) { var detail = HasDetailForProductSku(product.Sku) ? FindDetailByProductSku(product.Sku) : CreateDetailForProduct(product); return detail; } private OrderDetail CreateDetailForProduct(Product product) { var detail = new OrderDetail() { Product = product }; this._details.Add(detail); return detail; } private OrderDetail FindDetailByProductSku(string productSku) { var detail = this.Details.Single(d => d.Product.Sku == productSku); return detail; } private bool HasDetailForProductSku(string productSku) { return this.Details.Any(d => d.Product.Sku == productSku); }
At this point, the AddToCart() method has gone through some pretty extensive refactoring. The basic algorithm has been changed, and the implementation of the new algorithm has been changed a lot. Now let me point something out: At no time during any of these changes did our test fail, and at no time during these changes did our test fail to express the intended behavior of the class. We made changes to every aspect of the implementation: We changed the order of the steps in the algorithm. We constantly added and renamed methods until we had very discrete well-named functions that stated explicitly what the code is doing. The unit test remained a valid expression of intended behavior despite all of these changes. This is what it means to say that a test is more about API than implementation. The unit-test should not depend on the implementation, nor does it necessarily imply a particular implementation.
Happy Coding!