Ruby on Rails has inspired a whole generation of MVC Web Frameworks. The RoR inspired frameworks follow the same basic design; Controllers whose public methods are web endpoints. So…Do RoR based MVC Web Frameworks encourage good design?
They must. Right? I show, in the code below, that RoR inspired frameworks can actually encourage bad design. I show, in the end, what I think is a better way.
The code presented here is written for MonoRail, but the same problems exist in MS MVC and RoR.
Let’s start with a simple ProductsController
. The index method of this controller is the implementation of http://whocares.com/products/<code>
. The code on the end of the URL is the code for a specific product (i.e. nj1287).
public class ProductsController : SmartDispatcherController{ public void Index(string code) { Product product = Product.FindByCode(code); if(product == null) { PropertyBag["UnknownSearchTerm"] = code; RenderView("common/unknown_quick_search"); } PropertyBag["Product"] = product; RenderView("display_product"); }}
CategoriesController
is our next controller. The index method of this controller is the implementation of http://whocares.com/categories/<name>
. The name on the end of the URL is the name for a specific category (i.e. montiors).
public class CategoriesController : SmartDispatcherController{ public void Index(string name) { Category cat = Category.FindByName(name); if(cat == null) { PropertyBag["UnknownSearchTerm"] = name; RenderView("common/unknown_quick_search"); } PropertyBag["Category"] = cat; RenderView("display_category"); }}
The index method in both classes are basically the same. We can remove the duplication. Let’s extract an abstract class.
public abstract class SearchableController : SmartDispatcherController{ public void Index(string name) { ISearchable item = FindByName(name); if(item == null) { PropertyBag["UnknownSearchTerm"] = name; RenderView("common/unknown_quick_search"); } PropertyBag["Item"] = item; RenderView(IndexView); } protected abstract ISearchable FindByName(string name); protected abstract string IndexView { get; }}public class ProductsController : SearchableController{ protected ISearchable FindByName(string name) { return Product.FindByCode(name); } protected string IndexView { get { return "display_product"; } }}public class CategoriesController : SearchableController{ protected ISearchable FindByName(string name) { return Category.FindByName(name); } protected string IndexView { get { return "display_category"; } }}
What do you think about our new DRY code?I don’t like it. Why?
- ProductsController and CategoriesController no longer “speak” to me.
- Inheritance. I really don’t like the template method pattern because of #1.
Can we fix this?We can use a DynamicAction in MonoRail. Let’s see that code.
public class IndexAction : IDynamicAction{ private ISearchableRepository repos; private string indexView; public IndexAction( ISearchableRepository repos, string indexView) { this.repos = repos; this.indexView = indexView; } public void Execute(Controller controller) { string name = controller.Params["name"]; ISearchable item = repos.FindByName(name); if(item == null) { controller.PropertyBag["UnknownSearchTerm"] = name; controller.RenderView("common/unknown_quick_search"); } controller.PropertyBag["Item"] = item; controller.RenderView(indexView); }}public class ProductsController : SmartDispatcherController{ public ProductsController() { DynamicActions["Index"] = new IndexAction( new ProductsRepository(), "display_product"); }} public class CategoriesController : SmartDispatcherController{ public CategoriesController() { DynamicActions["Index"] = new IndexAction( new CategoryRepository(), "display_category"); }}
I like this code better. We are no longer using inheritance to reuse code and the code “speaks” better to me. This code has two things that bug me.
- The controllers are really not doing much.
- DynamicActions are second class citizens in MonoRail.
How do we fix these issues? I don’t know how in the RoR inspired frameworks. I think we need another abstraction. Chris Ortman and I discussed the below code at HDC 07 (so he deserves at least part of the credit). I am not sure he agreed, but maybe this post will change his mind.
The code below is not apart of any framework. It is only an idea.
public class IndexAction : SmartDispatcherAction{ private ISearchableRepository repos; private string indexView; public IndexAction( ISearchableRepository repos, string indexView) { this.repos = repos; this.indexView = indexView; } public void Execute(string name) { ISearchable item = repos.FindByName(name); if(item == null) { PropertyBag["UnknownSearchTerm"] = name; RenderView("common/unknown_quick_search"); } PropertyBag["Item"] = item; RenderView(indexView); }}
IndexAction
is now a first class citizen of the framework. Actions have all the benefits that controllers have in MonoRail (parameter binding, …).How do we hook this action to a URL?
new Route("/products/<name>", new IndexAction(new ProductRepository(), "display_product"));new Route("/categories/<name>", new IndexAction(new CategoryRepository(), "display_category"));
Routing engines allow us to remove the “controller” completely. We don’t need a “controller” (at least not in the RoR framework form). What were our controllers doing? They were really just organizing our actions. Routing engines allow us to organize our actions without the “controller”.What do you think? I would really like some feedback on this topic. Does anyone else feel this pain with these frameworks?
Filed under: Blog | Tagged: Agile, C#, MonoRail, MS MVC, TDD |
Hi Adam,
This approach also has a name. It’s called the Command pattern 🙂
What’s nice about controllers is that actions are logically grouped and so it’s easy to do things like call one from another and attach filters at the controller level. Additionally, it is trivial to factor out common code into private controller methods.
I wonder whether your controller extract superclass refactoring example is really how most people would DRY this code up?
Cheers,
Andrew.
@Andrew
You are right. It is the command pattern.
I am not sure I understand how you would DRY this up using private methods. I want to DRY across controllers not in the same controller. It seems like private methods only work in the same controller.
AE
have you looked at Django’s Generic Views?
http://www.djangoproject.com/documentation/tutorial04/#use-generic-views-less-code-is-better
Adam,
I wasn’t suggesting that you could DRY this particular code up using private methods, merely that being able to share private methods across logically grouped actions was handy.
I was suggesting that using inheritance here could perhaps be overkill. In RoR, for example, I suspect there is very little use of controller subclassing. In this case, a simple mixin could do the job. Of course, that’s Ruby for you 🙂
Oh and as for the Command pattern idea it’s a good one and quite popular already, particularly in Java land. Check out WebWork for one example.
Cheers,
Andrew.
Andrew,
I don’t think a Mixin works here either. A Mixin is simply a way to copy a method(s) to the controller. I am pretty sure RoR has the same issues I am pointing out here.
You can see more explanation about these issues in the castle dev discussion group.
http://groups.google.com/group/castle-project-devel/browse_thread/thread/976170df54fde8f5
AE
Hi Adam,
I am interested to know what you would think about using MVP with Commands instead. I don’t particularly dislike MVC, but I’m wondering if Microsoft really needs to kick everything out and start from scratch to give us the features we really want.
I actually think that the controllers will lead to worse code, as I can see people mixing up LINQ statements, presentation code, and all kinds of loose stuff in the PropertyBag.
Plus, the controller has code for all pages pertaining to a model, not just for one page. I don’t find that necessary nor helpful.
I am thinking of building a simplified version of the Patterns and Practices MVP pattern, and making it available on CodePlex. What do you think of that? Would it be useful?
I’d appreciate your comments, as I’m currently involved in blog-discussions with Scott Hanselman and Rob Conery about this very subject.
Best regards,
Richard
Richard,
I think MVC and MVP can both be used with single action controllers (commands/actions). In fact, I think that in order to maximize maintainability and DRYablity single action controllers should be used in most cases.
Do I think a simplified MVP pattern would be useful? Maybe. I think the best thing that can be done is to get out the message that multi-action controllers are not the best way to build large maintainable web applications.
Are we on the same page? Or have I totally missed your point.
AE