Is an abstract CRUD controller a good idea?

Question

We're developing quite a big application using ASP.NET MVC and at the beginning we saw that could be useful to have an abstract base controller with common CRUD actions (new, save, delete...) plus a default list action. In our case we have 20+ entities that are managed through this kind of controllers.

That works and avoids duplicating some code and makes the application more homogeneous but when you see a Controller is difficult to see exactly which actions does it implement and it may implement some actions that should not exist. And for example, imagine you want to edit passing the name and not the id, you have to create a new EditByName(name) and even doing that, you still have the Edit(id) action available because it's in the base.

To me the whole thing smells a little bit to me but I've not found any example showing an alternative because the MVC applications I see have a pretty narrow domain. Any advise? Any example? (I does not necessarily be in ASP.NET MVC, the problem I think is quite generic to any MVC framework).

Answer 1

This is one of the things Rails packs with ( along with generating the relevant CRUD views for you ) and it's great for whipping up a very quick application but once you get serious about development it is very unusual for the vanilla CRUD stuff to stay in because the things you need for each table start to change and need further customisation.

That isn't to say it doesn't form a useful foundation and a convenient way of setting things up while you're developing and prototyping - much easier than tinkering about in database fields - but certainly the way it works with Rails you don't expect to have any of those pages in your final administration system.

Answer 2

In some respects I think this is a good idea, but in others I think that it's an abuse of inheritance. I do have a common base controller, but it exists because I've refactored common code from my controllers into it rather than being designed a priori. If your entities are enough alike that the code that you're sharing in the base controller outweighs the cruft that you're forced to drag around, then perhaps it's worth it. If, on the other hand, the code that's being shared is rather minimal and simply invokes a private abstract method that does the work (so that you're implementing it in the real controller anyway), I don't know what it buys you. It's not like you're instantiating your controllers directly anyway (except in your tests) so having a common interface beyond that required by the framework isn't all that important.

My vote would be to refactor into the base class those things that are truly common or are cross-cutting concerns and not try to force an "is-a" relationship that may not really exist.

Answer 3

I created a version of what you're suggesting (though admittedly relatively non OOP) and it's worked very well for me.

I've created a MasterController that sets up a db instance and a few other variables. Once I started looking at the similarities in my CRUD actions, I realized this could be abstracted and moved into a method within the master. Two methods, actually.

protected ActionResult DisplayValidateAndEditModel<TModel>(TModel model, string modelPrefix,
                                        string editViewName, string successActionName, object routeValues, string successMessage,
                                        string[] includeProperties, bool acceptFiles
                                ) where TModel : class

and

protected ActionResult DisplayValidateAndEditModel<TModel>(TModel model, string modelPrefix,
                                        string editViewName, string successActionName, string successMessage,
                                        string[] includeProperties
                                ) where TModel : class

The Edit covers Create/Read/Update and Delete is Delete. Listing is a single line in a controller -- i just get a group of models and add to viewdata.

Both methods check if it's a post. If not, they return the view. If so:

• edit calls TryUpdateModel and also does some xVal validation. If all is ok, it redirects to the successAction with any routeValues. If not, it shows the view again. includeProperties can be passed so that my controller can specify exactly what can get updates. And acceptFiles adds additional functionality where it looks for a file posting and, if there, puts it in the database and creates a link between the file record and the model.

• delete updates the model's Cancel_Date and Cancel_User properties (I have a ICancelable interface) and redirects to the success action

Answer 4

To me the whole thing smells a little..

The same smell here :-)

It is common pratice to have little code in the controller. Its main purpose is to decide what View is rendered with which Model next.

If you wish to have a common place for all the controller code it shows, that you probably have to much logic in the controller.

Try to move the logik into the Model. Use Filters and CustomModelBinder.

Answer 5

Just a though: As you know ASP.NET MVC supports basic scaffolding like Ruby on Rails. What you may consider is to customize the T4 files used to create the Create, Update, Details Views to meet your specific needs.

http://blogs.msdn.com/webdevtools/archive/2009/01/29/t4-templates-a-quick-start-guide-for-asp-net-mvc-developers.aspx