Controller with too many dependencies?

Question

The main impetus for this review is to determine if my ASP.NET Web API controller has too many dependencies. I'm of the opinion that 3-4 is okay, but 6 seems like too many. I have many controllers with similar layout. I've considered extracting all of the "PostPrimary" operations into a single wrapping dependency, but not sure if that's overkill. If I did create a wrapper, I'm not sure whether I'd create some IManipulator interface and the wrapper could just hold its own IEnumerable<IManipulator> that it could iterate through and execute each, or whether It would just explicitly hold one of each dependency.

Some other things to note:

• This particular controller only has one "PrePrimary" manipulator, though similar controllers have more, so a PrePrimary wrapper would also be a possibility.
• I'd prefer to use some ORM instead of the repository pattern, but this legacy database is so messed up, NHibernate struggled to navigate my last project through the swamp of SQL, but the going was slow and they got bogged down many times.
• I understand that my repository's method contains waaay too many parameters, but I feel like a repository should be fairly open and honest about what constraints its going to use in its SQL, so I use a 1:1 relationship between repository parameters and SQL parameters that go into the WHERE clause. Is this reasonable?
• The verbosity of my identifiers is not open for discussion :P

So with that,

using System.Web.Http;
using MyProject.Models;
using MyProject.Models.DtoV1;
using MyProject.Models.Repo;

namespace MyProject.Controllers.Version1
{
    public class PropertyDimensionController : ApiController
    {
        private readonly IPropcodeVetter propcodeVetter;
        private readonly IPropertyDimensionRepository propertyDimensionRepository;
        private readonly IPropertyDimensionAllPropsManager propertyDimensionAllPropsManager;
        private readonly IComparisonRundateVsPopulator comparisonRundateVsPopulator;
        private readonly IMetricBenchRatePopulator metricBenchRatePopulator;
        private readonly IDecimalRounder decimalRounder;

        public PropertyDimensionController(IPropertyDimensionRepository propertyDimensionRepository, IPropcodeVetter propcodeVetter,
            IPropertyDimensionAllPropsManager propertyDimensionAllPropsManager, IMetricBenchRatePopulator metricBenchRatePopulator,
            IComparisonRundateVsPopulator comparisonRundateVsPopulator, IDecimalRounder decimalRounder)
        {
            this.propertyDimensionRepository = propertyDimensionRepository;
            this.propcodeVetter = propcodeVetter;
            this.propertyDimensionAllPropsManager = propertyDimensionAllPropsManager;
            this.metricBenchRatePopulator = metricBenchRatePopulator;
            this.comparisonRundateVsPopulator = comparisonRundateVsPopulator;
            this.decimalRounder = decimalRounder;
        }
        public PropertyDimensionResponse Post(PropertyDimensionRequest propertyDimensionRequest)
        {
            // PropertyDimension's PrePrimary Manipulations
            var vettedPropcodes = propcodeVetter.GetVettedPropcodes(propertyDimensionRequest);

            // PropertyDimension's Primary Operation
            var propertyDimensionResponse = propertyDimensionRepository.Read(propertyDimensionRequest.Clientcode,
                propertyDimensionRequest.Projectcode, vettedPropcodes, propertyDimensionRequest.Rundate,
                propertyDimensionRequest.ComparisonRundate, propertyDimensionRequest.DemographicResponse,
                propertyDimensionRequest.Metric, propertyDimensionRequest.MetricOnly);

            // PropertyDimension's PostPrimary Manipulations
            propertyDimensionResponse = propertyDimensionAllPropsManager.EnsureAllProperties(propertyDimensionResponse, propertyDimensionRequest);
            propertyDimensionResponse = comparisonRundateVsPopulator.Populate(propertyDimensionResponse);
            propertyDimensionResponse = metricBenchRatePopulator.Populate(propertyDimensionResponse);
            propertyDimensionResponse = decimalRounder.RoundAllValues(propertyDimensionResponse);

            return propertyDimensionResponse;
        }
    }
}

Answer 1

I'm going to make some high-level suggestions about the design. Generally, what I find helpful is to clarify what each class's responsibility is. From what I understand of your code, you have 3 types of objects:

• PropertyDimensionRequest: a data container which I assume contains the data of the web request.

• PropertyDimensionController: this does 2 things - loads data from database, and then does an "assembly-line" kind of processing on the request and response to populate the response.

• IPropertyDimensionAllPropsManager,IComparisonRundateVsPopulator, etc: which all populate parts of the web response.

Assuming I understood the function of these objects, I would make the following changes:

1. Modify PropertyDimensionRequest to retrieve the data and return the response. I think this better encapsulates the database parameters needed. (Why does PropertyDimensionController care what parameters are needed?) This allows gives you flexibilty to change the Data Access method, without affecting the Controller code at all.

2. Refactor the IPropertyDimensionAllPropsManager,IComparisonRundateVsPopulator, etc. to have a common interface, (maybe something called IResponsePopulator?), and add them to a list in the PropertyDimensionController class, or if they are reusable, and thread-safe, register them into some Singleton data structure.

The final code would look something like this:

  public PropertyDimensionResponse Post(PropertyDimensionRequest propertyDimensionRequest)
    {
        // not sure what I would do about this
        var vettedPropcodes = propcodeVetter.GetVettedPropcodes(propertyDimensionRequest);

        // Let PropertyDimensionRequest read from database, since it contains
        // all the parameters. Just pass it objects it doesn't have access to
        var propertyDimensionResponse = propertyDimensionRequest.Read(propertyDimensionRepository, vettedPropcodes);

        // this replaces all the code where the response was being
        // processed by various objects. If refactored to a common interface
        // you just load (or inject) them into a list, and then iteratively
        // process the response
        ResponsePopulators.ForEach( i => i.Populate(propertyDimensionResponse));

        return propertyDimensionResponse;
    }
}

(My C# coding is a little rusty, so please forgive any syntax errors).

Answer 2

Well Firstly that constructor is a bit of a worry.

Anything more than 3 (at most!) arguments is normally a bad idea.

Now that you have so many it would be a good idea to make yourself a :

    public interface PropertyDimensionControllerParameters : ApiControllerParameters
    {
        IPropcodeVetter vetter;
        IPropertyDimensionRepository repository;
        IPropertyDimensionAllPropsManager propsManager;
        IComparisonRundateVsPopulator comparisonRundateVsPopulator;
        IMetricBenchRatePopulator metricBenchRatePopulator;
        IDecimalRounder decimalRounder;
    }

which will clean up your constructor call and allow a much easier one-stop-shop functionality change. So now your constructor goes:

   public PropertyDimensionController(PropertyDimensionControllerParameters parameters)
   {
      if(parameters == null) throw new ArgumentNullException("parameters");
      _parameters = parameters;
   }

And you can do that across the board. Any where a method overload is super hard to read, encapsulate it in an object that makes sense.

Answer 3

I would consider inheritance when dealing with "I have many controllers with similar layout".

When dealing with constructor overinjection, you could consider refactoring to aggregate services.

I have also seen solutions where people use partial classes when dealing with constructor overinjection, wich I strongly wouldn't recommend in cases when you dont have automatic genaration of code.