views:

643

answers:

7

We are building a fairly large HR application in ASP.NET MVC, and so far our controllers are becoming quite large. For example, we have an Employee controller, and all employee views are included (Personal info, employee deductions, dependents, etc). Each of these views might have multiple actions or subviews (e.g. CRUD). Each action is relatively small, but the controllers might have dozens of functions.

Are there any best practices for splitting controllers? Instead of having an Employee controller with dozens of views, would it be better too have one controller for each subtype (i.e. EmployeePersonalInfoController, EmployeeDeductionController, EmployeeDependentController)?

And finally, does it even matter?

Updated Clarification

My original concern was with CRUD actions. For example, let's consider Create and Delete ...

Current Actions in EmployeeController:

  CreateEmployee()
  DeleteEmployee()
  CreateEmployeeDeduction()
  DeleteEmployeeDeduction()
  CreateDependent()
  DeleteDependent()
  etc.

If the controllers were split:

  EmployeeController
    Create()
    Delete()
  EmployeeDeductionController
    Create()
    Delete()
  EmployeeDependentController
    Create()
    Delete()
  EmployeeBenefitController
    Create()
    Delete()
  etc.

In the 1st scenario, our ~100 screens get split into 8-10 large controllers. In the second, I'd probably have ~50 controllers.

+3  A: 

Why not group them?

Have a structure like,

employee/payroll/
    employee/payroll/giveraise
    employee/payroll/manage401k

employee/general/
    employee/general/address
    employee/general/emergencycontact

Now you can have one payroll controller handling payroll related actions and a general controller which handles regular details of an employee.

SolutionYogi
We'd love to, but in the standard ASP.NET MVC there is no concept of areas (which I believe is what you're suggesting). We could add an extension that supports areas, but I don't like adding a bunch of stuff if it isn't required.
Jess
I think what Yogi Means is payroll is the controller and giveraise/manage401k is the action of those. No need to have any extension, just add some more routing rules to the global.asax would works.
xandy
@Jess et al - FYI support for areas now exists: http://msdn.microsoft.com/en-us/library/ee671793.aspx
Drew Noakes
+2  A: 

In my humble opinion, if you are keeping the code in your controllers down then it doesn't really matter.

Most of your code would be happening in a business layer somewhere right? If that's the case then all you are really doing in your controller is returning data to the view. As it should be.

Not really sure if I'm a fan of seperating the controllers into subtypes. Whilst you should maintain seperation of concerns I think subtypes is going a little too far.

You could take a look at this post to see if it helps. Same View Different Paths

That may be a better solution than using a subtype approach that you suggested.

griegs
Not sure if "subtypes" was the best wording on my part. I wouldn't be implementing areas (grouping them into folders), just creating more of them. I'll update the question with some type of example to clarify. Thanks.
Jess
+1  A: 

Controllers are meant to be containers for actions under one context. I.E. a customer controller would have actions pertaining to controlling customers. This is particularly suited to CRUD. I would go with fewer larger controllers for this reason. That said, it is really up to you as the application architect to choose the way that best suits your code and just because it is more common to do it one way doesn't mean you have to.

If you have large amounts of code I would suggest you look into ASP.NET MVC areas. You can find excellent posts about it Here in Scott Gu's blog and Here in Steve Sanderson's blog. If you have so many controllers, it might be suitable for you.

Just had a thought after re-reading your post, I suspect your example doesn't come close to the level of complication you have in your code. Perhaps it might help if you posted a situation where you were unsure whether or not it was a good idea to split your controller that is more specific (and less CRUDDY, because CRUD is fairly straight forward).

Odd
+6  A: 

Partial classes allow you to spread your class across multiple files. That way you can group relevant areas of your controller into separate files, and yet they'll all still be part of the same controller. e.g.

EmployeeDeductionController.cs

public partial class EmployeeController
{
    public ActionResult Deduct()
    {
    }
    // etc
}

EmployeeBenefitController.cs

public partial class EmployeeController
{
    public ActionResult GiveBenefit()
    {
    }
    // etc
}
Sam Wessel
This can work quite well, it is about the only reason why I would bother with a partial class.
Mark Dickinson
Good thought ... but out of curiosity, how is this more beneficial than splitting the Controller up into many controllers?
Jess
An interesting idea, but I think it abuses the partial class feature a little bit. Partial classes were designed primarily for code generation scenarios so that developers could add to a generated class and have those changes be preserved during a re-gen. Unless I misunderstand, I think it's better to create separate controllers with a shared base class than to use partials.
Seth Petry-Johnson
Since posting my answer I actually completely agree with you Seth. Partial classes have bitten me in the past and I generally avoid them now. Oh how fickle I am.
Sam Wessel
+2  A: 

I would not want to have 50 controllers. Right now I have 16 in my application and that feels ok. If you have 50 controllers you will also have 50 toplevel folders for views. It will be hard to find the view and controller you need to work on. As others mentioned actions are typically short and its not that bad to have a couple of them in your controller.

I tried to have 1 controller by system part. I define a system part by taking my database schema and drawing a line around tables that belong together.

Malcolm Frexner
+1  A: 

I would organize the controllers roughly around the use cases and their logical grouping. E.g. if you have multiple administrative/HR-type use cases which are likely to be available to a limited group of people, bundle those in one controller. Other controllers could be organized around specific domain model objects - e.g. self-service leave management, salary queries etc. There's no hard and fast rule, you have to create a balance between not putting too much responsibility into a single controller vs. reuse of common internal structures.

Remember also that as much as possible you shouldn't have core business logic in your controllers. They really implement the front-end behavior while the real system rules should be in your domain model and service layer. As long as you keep things roughly within the right layer and reasonably decoupled, you can't go too far wrong with how you place the individual operations within your controllers.

Pavel
+1  A: 

Another approach we've been using is having a ControllerBase to keep cross-cutting concerns in a common place for CRUD operations. This controller declares the common operations and includes extension points for the specific entity stuff. We had too many code duplication without something like this.

Then, you inherit this controller and create one per entity. And yes, there are many controllers but having so many screens, I don't think it will be the main problem.

Most of the actions accept a complex Model and we play then with the model binders to remove clutter from the controllers. You can see a good post about that here.

Then, using areas like @Odd suggests is a good idea, at least to separate the views because when you have a lot of them is a mess.

Hopefully ASP.NET MVC v2 will bring us areas and encapsulating views in different assemblies (actually that can be done now extending the VirtualPathProvider class).

Marc Climent