3
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

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).

\$\endgroup\$
3
  • \$\begingroup\$ Interesting. The PropertyDimensionRequest is just a DTO, so I felt compelled to keep it logic-free and as simple as possible. Of course Read() could be an extension method on it, but then I don't get to easily mock it, so I'd use a repository method, and then we're back where we started, although I'd just pass the entire request into the repository and get back the skeleton of the response. The rest of your thoughts on the collection of populators is kind of what I was thinking with regards to my manipulator wrapper, except you're suggesting forgoing the wrapping. \$\endgroup\$ – mo. Feb 5 '14 at 20:40
  • \$\begingroup\$ @mo.: I did a little reading on the Repository pattern. One point to take from this article, is that you could remove the dependencies between the Controller and the specific PropertyDimensionRequest properties used for the data query, by adding an "Entity specific" look up method. (See the HotelRepository.FindHotelsByCity() method in the article). Adding the specific lookup function to your Repository provides a better separation of concerns, than leaving it in controller. \$\endgroup\$ – Sam Goldberg Feb 6 '14 at 17:45
  • \$\begingroup\$ Note that this specific code wouldn't work, because all the Populate() methods return the changed response. But that's trivial to fix: foreach (var populator in ResponsePopulators) propertyDimensionResponse = populator.Populate(propertyDimensionResponse);. \$\endgroup\$ – svick Feb 18 '14 at 14:48
2
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ I've heard of making parameter classes for certain things, but never for a constructor. Wouldn't using aggregate services (as named by Dzenan and originally hinted at by myself) make a lot more sense in this case? \$\endgroup\$ – mo. Feb 6 '14 at 15:08
  • \$\begingroup\$ Was trying to edit, but took so long my time limit expired :O - It seems like your suggestion would be sweeping the too-many-concerns problem of the controller under the rug. \$\endgroup\$ – mo. Feb 6 '14 at 15:16
  • \$\begingroup\$ Well I am a major proponent of Dependency Inversion. All of my services are written as interfaces and passed into things via constructors. that way regardless of what implementation method you choose, one that requires 4 items or 40 the function calls remain the same and no code needs recompiling. I don't hold constructors as sacred as most do. I tend to build things via factories so they don't matter, the implementation classes are never exposed. all my implementations are internal while interfaces are public. In short it doesn't matter which way you go, as long as you code to a contract \$\endgroup\$ – apieceoffruit Feb 6 '14 at 15:18
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ The inheritance idea is interesting, but I feel the duplicated logic is so minute that it probably wouldn't be worth the overhead. \$\endgroup\$ – mo. Feb 5 '14 at 20:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.