In the world of ASP.NET with Dependency Injection, the rule for how many parameters is too many parameters doesn’t really work all that well when it is applied to a constructor.
I whole heartedly agree with it as a rule to be applied to conventional methods, but as a rule for constructors it seems to contradict the dependency injection pattern.
There are many arguable cases where having more than 7 parameters in a constructor is a valid use case - such as manager layers in ASP.NET APIs.
Is there a way that this rule could be isolated to parameters only and not the constructors?
Why do you think a class should have that many dependencies? If a class (DI or not) has that many, I would argue that is time to reconsider it’s responsibilities, as it probably has too many. (And I think, that is what this rule is about)
Hey @Corniel - I would usually agree with you completely.
In my case in particular, we introduce many helpers which we invoke from our manager. These helpers exist to allow us to do things like implement adapters to convert from API requests to internal models that we actually interact with.
Furthermore, our API is robustly built with feature managers for new features we release which is integrated with LaunchDarkly. Essentially, with every new feature, where there may be business specific-logic to our application, we will introduce helpers that manager the new feature we are developing. We then pass on API manager layer to these helpers and take those results and either read/write to a DB. These helpers, aside from managing techniques such as feature toggles, also have redundancy built-in with retry policies and circuit breaker patterns so as to fail transiently without bringing down our surrounding ecosystem and retry when the ecosystem becomes available.
As a result, our manager ends up with a handful of dependencies for the database calls, along with dependencies for the helpers for all our features.
So in 99.99% of our development, we follow the rule strictly, in this particular case the rule doesn’t work for us and we are finding this is becoming more and more common as we develop software in a way that allows to safely separate deployment from release. I am sure that there are other teams out there facing this issue as we expand our DevOps capabilities.
Just wondered if it’s possible to configure/split this particular rule for C# to be 1 for constructors and 1 for method parameters.
To prevent any misunderstanding, I’m not working for SonarSource, I’m just a happy coder (that have done some contributions to the Sonar .NET analyzers). So, it my personal opinion I’m sharing here.
To me, having one exception to the rule, is not a reason to abandon it. It is reason to document why, in this particular case, you intentionally violated it. So, that your later self, or (even more likely, you future colleague) can challenge, or re-evaluate the decision made.
That being said, It might be worth, to group some of these dependencies related to those feature managers, and inject them at once. That obviously only helps if, and only if, that does not increase the number of violations.
Personally I would like to see this rule “split” into two, either two separate rules or just a rule with multiple thresholds. So we could have separate values for methods and constructors, and possibly separate severity levels.
I agree this needs to be two separate rules - one for constructors and one for methods.
I have constructors with 30+ parameters simply because the class is an immutable model that has a lot of data properties and we I use a data layer that can de-serialize the results of SQL queries into immutable models using constructor injection.
If you won’t split it, at least allow the two thresholds to be set separately in sonar cloud.
Sorry for the delay on this one. This thread never made it to the right’s team’s desk (and this thread started before we had a process for doing so). I’ve flagged it for attention.
I just turned the rule on for one of our larger projects (520,000 lines of source and the default 7 parameter limit) and got 272 hits for constructor arguments and 72 for method arguments. Of the 272 constructors, 240 were immutable models, and 31 were dependency injections for services (which weren’t far over the limit) and 1 was something else.
My primary concern is not so much the number of instances in a particular code base but rather the message it sends to junior developers. If the company policy was to have this rule on and it generates a message that having lots of constructor parameters is a poor code style, they would then reason “well immutable models need lots of constructor parameters, so using them must be poor coding style” whereas using immutable models is excellent style that we want to encourage as much as possible.
But if we have the rule off then they don’t get warned about methods with too many arguments, which is often poor coding style, and should be reviewed during PRs.
The problem with Won’t Fix is if the developers see the issue via sonar analyser in visual studio they change the code to something that is actually worse style before submitting if for pull request review where it would get picked up in sonar cloud and marked Won’t Fix.
If it were split into two rules, we could have the method one on and the constructor one off.
Alternatively if the analyser could detect it was an immutable model (no property sets, no public fields, no non-pure methods) and not fire the rule that would be even better. The last one I suspect is tricky though. Not firing if there are no property setters, no public fields and no methods would help though.
Where the cause of too many constructor parameters is too many dependency ingestions, it is worth reviewing as sometimes that is a violation of the single-purpose class principle but sometimes is valid if it is a co-ordinator service.
I think it should be possible to exclude all arguments that are directly set to read-only properties/fields, or take that in to account differently, as you suggested.
For constructors used by DI that will be trickier. But as @jonataspc suggested, by splitting the rule in two, you could parameterize the number of arguments for a constructor differently in projects that use DI, without making the rule less strict on methods.
@paulhickman What do you consider a feasible maximum for constructors used by DI?
