Python: Keyword arguments and "Functions, methods and lambdas should not have too many parameters

Clean Code Report False-positive / False-negative...
,
1

In my opinion this rule should ignore mandatory named arguments that have been separated in the function definition by a * in the argument list.

Noncompliant Code Example

With a maximum number of 4 parameters:

def do_something(param1, param2, param3, param4, param5='default value'):
    pass

This is non-compliant because the function can still be called like this:

do_something(1, 2, 3, 4, 5)

Compliant Solution

def do_something(param1, param2, param3, param4, *, param5='default value'):
    pass

In this case the * marker in the function definition dictates that the param5 parameter should always be passed as a named argument. Therefore the maximum number of positional parameters for this function is 4 and should be compliant as named parameters self document the method call and cause less brain-overload.

This should be compliant because the function can only be called like this:

do_something(1, 2, 3, 4, param5=5)

This could either be implemented in this rule or added as an additional rule, then the maximum total number of parameters can also still be validated. Maybe it makes sense to also limit the maximum number of mandatory named arguments using a rule configuration setting.

I’ve reported this as a comment on https://jira.sonarsource.com/browse/RSPEC-2671?focusedCommentId=214230&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-214230 as well.

3

Hi @sjaak,

Thank you for your suggestion and for taking the time to move it to this forum :slight_smile:

Before answering let me first make sure I understand what you mean by “mandatory named arguments”. For me an argument is mandatory if it has no default value. A mandatory keyword-only parameter would look like this:

def foo(*, mandatory_param):
    pass

Yet in your example param5 has a default value so I might be missing something.

4

It should hold also for a parameter that has no default value. The “mandatory keyword” name I chose to describe this relates to the fact that you must pass it as a keyword argument. You are right, this is ambiguous, I should have used the name “keyword-only argument”.

I think the point of this rule is to prevent brain overload, which is the case if you need to remember what argument position maps to which argument. But if the call is made with keyword arguments that makes it a lot clearer what is being passed to the function. Therefore it would be nice if sonarqube could distinguish between this case and just a plain methods with a large number of possible positional arguments.

5

Hi @sjaak,

I hope you are well in these trying times.

You are right that this rule is about brain overload, that calling a method/function with too many positional parameters is difficult to understand, and that naming them makes it better.

I feel however that we might be mixing two different things here: function definition and function call. After reading your post a few questions came to my mind, the first one being…

1/ Is the rule raising false positives?

Before going for a specific solution, let’s first clarify the problem. I took a look at how this rule behaves on some well known open-source projects. The maximum number of parameters was left to its default value, i.e. max 7 parameters. Some projects had less than 10 issues but many big projects (pandas, scikit-learn, tensorflow) had more than 100. Raising that many issues indicates that there is clearly a problem with either the pattern detected by rule S107, or its default configuration.

2/ Would Ignoring keyword-only parameters solve the problem?

Let’s see if keyword-only parameters should be ignored. We can do this by digging a bit in these open-source projects and see if they use keyword-only arguments.

It seems that we would still raise most issues if we ignored keyword-only parameters. This makes sense as many projects still had to support python 2 in 2019. I doubt that they will change their APIs anytime soon.

3/ Are there alternative solutions?

I did a rapid search to see how the 34-parameter-initializer of Tornado is used. Developers have the common sense of passing arguments by name rather than position. We probably don’t have to force them to do it. Actually, keyword only arguments were not created to enforce the use of named arguments. They were added to improve the use of varargs. Of course there are other benefits such as the impossibility to pass an argument as positional and keyword at the same time.

This also matches python philosophy: “We are all responsible users”. Developers are not expected to go out of their way to forbid improper use of a feature.

So how do we make sure that:

  • We don’t raise False Positives with this rule, i.e. everybody agrees that the pattern it detects is wrong.
  • This rule works out of the box for most people, the few remaining projects just have to configure it.
  • This rule is simple to understand and to follow (as Zen of python says “Simple is better than complex.”)

Maybe we could consider that the problem of passing too many positional arguments to a function call is a separate problem.

I suggest the following:

  • For rule S107:
    • Set the default value for the maximum number of parameters to a higher value. I don’t know which value yet, this needs to be tested. I would be interested in knowing the type of project you work on and the max number of parameters you would use.
    • Improve the description to make it clear that this rule does not focus on how the method is called, but rather how it is designed and how well it is understood by developers. Something along the lines of:

Python functions and methods methods should not have too many parameters, even if they have default values, because:

  • it makes their signature difficult to read and document. You can group parameters into structures to make relations between them clearer.
  • a function with many parameters usually does too many things. If this is the case you should probably refactor the code into multiple functions/classes/methods, each having clear responsibilities.

This rule raises an issue when a function or method has too many parameters.

  • Add a separate rule to detect when developers call a method or a function with too many positional parameters. It would recommend to pass these parameters by name.
    • I am interested in the maximum number of parameters you would use.

We can also envision a rule which recommends the use of keyword only parameters when a function has too many parameters with default values. However this rule would have to be optional as it would raise many issues on Django and other well known projects. We first need to check if it is a best practice or not. All opinions on the matter are welcome.

Does it make sense to you? Feel free to point any flaw in my reasoning or suggest other solutions.

6

Hi Nicolas,

Sorry for the long wait for my reply, I haven’t been here in a while.

Thank you for your in-depth analysis of the issue, it all looks very good.

You are correct that my real issue is with the call to the methods and not being able to understand if there are too many positional parameters and not necessarily with the method declaration.

I think having a separate rule that validates calls to methods using too many positional parameters makes sense, that would already solve most of my issues with the existing rules. In my opinion 5 parameters in a method invocation is already a bit much if they’re positional, but I’m fine with setting it to 7 since that seems to be the number for the other rule.

7

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.