← Change is in the air... | Home | Quick Tip: Asserting response redirects in a MonoRail controller test →

Refactoring towards a DRY, fluent interface

posted @ Thursday, September 06, 2007 7:16 AM

 

But I (Jesus) say to you, love your enemies, bless those who curse you, do good to those who hate you, and pray for those who spitefully use you and persecute you -- Matthew 5:44

Problem

In my current project, I have an application service that is used to make various modifications to a shopping cart, like adding cart items, applying discounts, changing quantities, etc.  Well as this service class evolved, the methods were starting to look like this...

   1: public void ApplyDiscountToShoppingCart(decimal discount)
   2: {
   3:     IShoppingCart shoppingCart = shoppingCartRepository.GetCurrentShoppingCart();
   4:     shoppingCart.ApplyDiscountOf(discount);
   5:  
   6:     shoppingCartRepository.Save(shoppingCart);
   7: }
   8:  
   9: public void AddToCartUsing(string productId, int quantityToAdd)
  10: {
  11:     IShoppingCart shoppingCart = shoppingCartRepository.GetCurrentShoppingCart();
  12:     shoppingCart.AddItemToCart(cartItemFactory.CreateFrom(productId, quantityToAdd));
  13:  
  14:     shoppingCartRepository.Save(shoppingCart);
  15: }
  16:  
  17: public void ChangeQuantityForCartItem(Guid cartItemId, int newQuantity)
  18: {
  19:     IShoppingCart shoppingCart = shoppingCartRepository.GetCurrentShoppingCart();
  20:     shoppingCart.ChangeQuantityForItem(cartItemId, newQuantity);
  21:  
  22:     shoppingCartRepository.Save(shoppingCart);
  23: }

Notice the amount of duplication going on here?  We always need to retrieve the current shopping cart, perform some action on it and save it.  Smells like it's time for some refactoring.

(One) Resolution 

Important to note, that I've already fleshed out my application service class via TDD, with all unit tests currently passing.  So this refactoring is simply to remove duplication, and therefore improve the design (as opposed to a Test-Driven Refactoring which is often a very good way of making changes and/or adding new features). 

By definition, refactoring is the act of "altering the internal structure without changing the external behavior" through a series of "behavior preserving transformations".

So let's get to it.  First off, I pick a method to start with; let's choose ApplyDiscountToShoppingCart.  Just like when practicing TDD, I'm going to write out the syntax that I would like to be able to use.  Then, implement it.

   1: public void ApplyDiscountToShoppingCart(decimal discount)
   2: {
   3:     Within.ShoppingCartSaveScope(shoppingCartRepository).Do(delegate(IShoppingCart shoppingCart) { shoppingCart.ApplyDiscountOf(discount); });
   4: }

Ok, so that looks ok (although that anonymous delegate may need some more work, more on that in a sec).  Readability is pretty important to me, so basically what I'm saying here is that Within a ShoppingCartSaveScope, Do the action that I tell you to do.

First we'll need to create this thing called Within.  This will be the entry point to initiate the operation.  So I'll just make it a nested private class for now.

   1: private class With
   2: {
   3:     public static ShoppingCartSaveScope ShoppingCartSaveScope(IShoppingCartRepository shoppingCartRepository)
   4:     {
   5:         return new ShoppingCartSaveScope(shoppingCartRepository);
   6:     }
   7: }

Ok, pretty simple.  Just one problem.  We don't have this ShoppingCartSaveScope class yet.  So let's create it also as a nested private class (using ReSharper in all of this of course...).

   1: private class ShoppingCartSaveScope
   2: {
   3:     private readonly IShoppingCartRepository shoppingCartRepository;
   4:  
   5:     public ShoppingCartSaveScope(IShoppingCartRepository shoppingCartRepository)
   6:     {
   7:         this.shoppingCartRepository = shoppingCartRepository;
   8:     }
   9: }

Looks good.  Notice we'll have to pass it an instance of our IShoppingCartRepository which is currently being injected into our service class from an IoC container.  So back up to our original method.

   1: public void ApplyDiscountToShoppingCart(decimal discount)
   2: {
   3:     Within.ShoppingCartSaveScope(shoppingCartRepository).Do(delegate(IShoppingCart shoppingCart) { shoppingCart.ApplyDiscountOf(discount); });
   4: }

Now we have to deal with this Do method.  The method name is probably influenced by the time I've been spending writing Ruby lately.  So basically this is taking advantage of the wonderful world of "blocks".  I'm sure most of us know that in C# we can pass blocks of code as method arguments using an anonymous delegate.  But most of the time, folks are only utilizing this powerful feature doing things like List<T>.FindAll() or similar.  So what do we do when we need to write a method that consumes a code "block"?  Well, you have a couple options. 

Predicate<T> is what is used in most of the List<T> methods, like Find, RemoveAll, etc.  The point of a Predicate is to return a boolean value so that the caller can determine what to do based on the result. 

Action<T> on the other hand is simply a "fire and forget it" kinda thing.  It acts the same as the Predicate<T>, with the exception of returning a value. 

For now we're not going to worry about returning a boolean, so we'll just go with Action<T>.  So remember our Within class returned us back an instance of ShoppingCartSaveScope.  That's where we'll need to implement our Do method.

   1: private class ShoppingCartSaveScope
   2: {
   3:     private readonly IShoppingCartRepository shoppingCartRepository;
   4:  
   5:     public ShoppingCartSaveScope(IShoppingCartRepository shoppingCartRepository)
   6:     {
   7:         this.shoppingCartRepository = shoppingCartRepository;
   8:     }
   9:  
  10:     public void Do(Action<IShoppingCart> actionUsingShoppingCart)
  11:     {
  12:         IShoppingCart shoppingCart = shoppingCartRepository.GetCurrentShoppingCart();
  13:         actionUsingShoppingCart(shoppingCart);
  14:  
  15:         shoppingCartRepository.Save(shoppingCart);
  16:     }
  17: }

Notice our Do method simply takes an Action<IShoppingCart> which is just some code block that does some action on an instance of a shopping cart.  Here's where the removal of duplication happens.  Previously, we had to both retrieve and save the shopping cart in every single service method.  Now, we can put this in a single place, where it can be re-used by any operations that need to perform some series of actions on a shopping cart and then save.

Don't Like In-Line Anonymous Delegates?

I'm usually fine with using anonymous delegates in-line, but this is one instance where I felt something like this would be much more readable.

   1: public void ApplyDiscountToShoppingCart(decimal discount)
   2: {
   3:     Within.ShoppingCartSaveScope(shoppingCartRepository).Do(ApplyDiscountToShoppingCartCommand.Using(discount));
   4: }

This could simply be implemented by extracting out the anonymous delegate into its own method.  I'm using another private class for readability's sake, but you could just use a private method as well.

   1: private class ApplyDiscountToShoppingCartCommand
   2: {
   3:     public static Action<IShoppingCart> Using(decimal discount)
   4:     {
   5:         return delegate(IShoppingCart shoppingCart) { shoppingCart.ApplyDiscountOf(discount); };
   6:     }
   7: }

So now, just rinse and repeat with the rest of the service methods as necessary.

Any Of This Look Familiar?

Ok, so this whole ShoppingCartSaveScope deal is basically a very lightweight implementation of the Unit of Work pattern (without change tracking and all the other stuff you normally would need).  But this is all we need so far.  And it keeps our code DRY, while at the same time providing us with a decent fluent interface that we can use.

Better Solutions?

Again, this is just merely one of many ways we could have done this.  In fact, I'm thinking that it could be done even easier using Aspects (AOP), but I'll have to save (and learn) that for another time.

Feedback?

Comments
Joey Beninghove - 9/6/2007 4:55 PM
# re: Refactoring towards a DRY, fluent interface
Yeah, you certainly could. Although, the fact that the 'Do' method is performing a 'Save' is really only 1/3 of what that method is actually doing. It's also retrieving the current cart and running the specified action before save is called.

But as usual, naming conventions come down to personal/team style.
Joey Beninghove - 9/11/2007 3:37 PM
# re: Refactoring towards a DRY, fluent interface
Using MR filters would definitely be a good way to go, if my controllers interacted directly with my domain model (including repositories). But...

In this case, the domain model is shielded from the presentation layer through a thin service layer which only works with DTOs. And my IBasketService doesn't explicitly expose a "Save" method, only individual operations like "AddToBasket", "ApplyDiscountToBasket", etc. So since the "Save" has to happen on the Repository, I felt the solution above was a viable way of reducing duplication while gaining a decent syntax.

I too don't really like the extra classes all that much, but most fluent interfaces I've seen implemented (in C# anyways) usually require a handful of classes to gain the nice syntax. As I dig more into Ruby, I'm sure I'll find that this type of thing will be MUCH easier. Looking forward to my first Ruby user group meeting tonight. :)
Erik Lane - 9/12/2007 2:45 AM
# re: Refactoring towards a DRY, fluent interface
I will say that scenarios like this tend to get close to the gray areas of complexity for complexity sake. However, that's a moot point if the team is on board.

I like it though - nice, clean, and makes total sense for someone else reading the code. Nice job!
Joey Beninghove - 9/12/2007 6:46 AM
# re: Refactoring towards a DRY, fluent interface
Yeah, I can definitely see how it could be "complexity for complexity sake". That's why I've isolated it to this one class. It may work out, or it may not. My motiviation of removing duplication I believe is correct, but Maintainability is still "-ility" #1. So if this refactoring is deemed to make the code less maintainable and/or easy to understand, by me or my team, I have absolutely no problem in backing it out. After all, the *team* owns the code, not *me*.

And, if needed, I have no problem "changing my mind": http://www.37signals.com/svn/posts/595-aint-nothing-wrong-with-contradiction

:)
Erik Lane - 9/13/2007 11:51 AM
# re: Refactoring towards a DRY, fluent interface
I like your motivation and I like the technique that you used. It makes total sense to me. I only bring up the complexity comment because I've been put in that situation before. I'll do something (like remove duplication) and shown it to the team. Later I'll find other code that, instead of following the same design/style, is just a hack job to get around it...which makes it worse.

I'm fine with being wrong or backing out my code because it isn't needed but if the team doesn't get it and doesn't speak up...it can make it worse. woohoo! :-)
Colin Jack - 9/21/2007 7:35 AM
# re: Refactoring towards a DRY, fluent interface
Hrm, I like your approach but it does require a few extra classes and if you do this in every case then you are going to end up writing a lot of them. I guess this is because you are trying to build this fluent interface on top of your normal API.

I'm with a few other people here, AOP with something like PostSharp would seem to me to be a good approach to evaluate.
Joey Beninghove - 9/21/2007 9:17 AM
# re: Refactoring towards a DRY, fluent interface
@Colin,
Indeed. And this is actually the only place I'm doing it right now. Just to see how it played out. As I stated above, I have absolutely no problem with backing out of that implementation if necessary, and could be trivially done so since it's isolated.

If only I could find the time to actually learn AOP, I'm sure I'd be able to make this drop dead simple.
Post Comment






Please add 8 and 4 and type the answer here: