80

I want to refactor a big Python function into smaller ones. For example, consider this following code snippet:

x = x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9

Of course, this is a trivial example. In practice, the code is more complex. My point is that it contains many local-scope variables that would have to be passed to the extracted function, which could look like:

def mysum(x1, x2, x3, x4, x5, x6, x7, x8, x9):
    x = x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9
    return x

The problem is that Pylint would trigger a warning about too many arguments.

I could avoid the warning by doing something like:

def mysum(d):
    x1 = d['x1']
    x2 = d['x2']
    ...
    x9 = d['x9']
    x = x1 + x2 + x3 + x4 + x5 + x6 + x7 + x8 + x9
    return x

def mybigfunction():
    ...
    d = {}
    d['x1'] = x1
    ...
    d['x9'] = x9
    x = mysum(d)

but this approach loos ugly to me. It requires writing a lot of code that is even redundant.

Is there a better way to do it?

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Anonymous
  • 3,011
  • 4
  • 24
  • 23
  • 1
    I believe mysum() could be simplified to: 'return sum(d.values())' or at least 'return sum([d[foo] for foo in ('x1', 'x2', ..., 'x9')])'. Am I being too list-comprehension happy? – Tyler May 03 '09 at 05:24
  • 11
    The mysum() is just an abstraction, in real scenarios the code that needs to be extracted is much more complex. My point is about having to pass many variables to the extracted function and avoiding the pylint warning if possible (without explicitly making pylint to just ignore that warning). – Anonymous May 03 '09 at 05:27

11 Answers11

160

First, one of Perlis's epigrams:

"If you have a procedure with 10 parameters, you probably missed some."

Some of the 10 arguments are presumably related. Group them into an object, and pass that instead.

Making an example up, because there's not enough information in the question to answer directly:

class PersonInfo(object):
  def __init__(self, name, age, iq):
    self.name = name
    self.age = age
    self.iq = iq

Then your 10 argument function:

def f(x1, x2, name, x3, iq, x4, age, x5, x6, x7):
  ...

becomes:

def f(personinfo, x1, x2, x3, x4, x5, x6, x7):
  ...

and the caller changes to:

personinfo = PersonInfo(name, age, iq)
result = f(personinfo, x1, x2, x3, x4, x5, x6, x7)
w43L
  • 565
  • 1
  • 7
  • 26
  • 24
    I like this answer because it points out how to think about the problem! – Frank M Nov 12 '14 at 18:04
  • 2
    Note that from Python 2.6, you can use collections.namedtuple to do the exact same thing instead of class. – Ignatius Feb 18 '19 at 06:55
  • 2
    Ultimately, the best answer is the one that meets the OP's needs, however this certainly should have been / should be selected because the others tell you how to get around the warning, whereas this answer tells how to restructure the parameters to meet the author's needs. – demongolem Nov 13 '19 at 14:13
  • 1
    Maybe it's just me, but writing a whole class just to combine a few parameters (possibly only for one specific function) seems quite overkill? – Lu Kas Sep 26 '22 at 14:33
  • If you do this pylint will complain `Too few public methods` because classes aren't suppose to just hold data. Maybe this is what data classes are for? https://docs.python.org/3/library/dataclasses.html – red888 Feb 02 '23 at 19:56
78

Do you want a better way to pass the arguments or just a way to stop Pylint from giving you a hard time? If the latter, you can stop the nagging by putting Pylint-controlling comments in your code along the lines of:

#pylint: disable=R0913

or, better:

#pylint: disable=too-many-arguments

remembering to turn them back on as soon as practicable.

In my opinion, there's nothing inherently wrong with passing a lot of arguments and solutions advocating wrapping them all up in some container argument don't really solve any problems, other than stopping Pylint from nagging you :-).

If you need to pass twenty arguments, then pass them. It may be that this is required because your function is doing too much and refactoring could assist there, and that's something you should look at. But it's not a decision we can really make unless we see what the 'real' code is.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
paxdiablo
  • 854,327
  • 234
  • 1,573
  • 1,953
  • 10
    This answer encourages really bad habits - read now in 2015. 1) disabling checks with their number - modern pylint versions support symbolic descriptors, much more explicative 2) disabling something should have also a comment with an explanation of the reason why it's being disabled 3) there should be only one directive per line, balanced by a corresponding re-enabling 4) Usually pylint does have a point. Code that clashes with those limits is probably unreadable/unmaintainable. 5) Rather than passing a deluge of parameters, it's much better to use a dictionary or a named tuple. – Igor Stoppa Feb 12 '15 at 19:40
  • 10
    Igor, that's why I said "along the lines of". By all means use symbolics rather than numerics, or one per line, or comment why, or re-enable (though you should consider what that means if it was previously disabled - better would be a method to save/disable/restore). None of that changes the utility of the answer itself, which is to tell pylint to stop warning about a problem the user _knows_ they don't want to hear about (and accepts the consequences). – paxdiablo Feb 12 '15 at 22:12
  • 2
    The user most likely is often in self contradiction: pylint is not the most explicit tool in that sense, but the warnings given usually have far deeper reach than what the message seems to give. So the user might *think* to know what consequences are accepted, but (s)he doesn't fully appreciate the implications. – Igor Stoppa Feb 13 '15 at 11:30
  • 6
    Wether good or bad habit, this is the answer to the question!! – SeF Aug 20 '20 at 16:49
  • Re *"by putting Pylint-controlling comments in your code"*: Where exactly (in the beginning of a file? In the beginning of a function? Anywhere)? And what is the scope (the rest of file? the rest of the function? Something else)? Can you update your answer? – Peter Mortensen Sep 18 '21 at 15:47
48

You can easily change the maximum allowed number of arguments in Pylint. Just open your pylintrc file (generate it if you don't already have one) and change:

max-args = 5

to:

max-args = 6 # Or any value that suits you

From Pylint's manual

Specifying all the options suitable for your setup and coding standards can be tedious, so it is possible to use a rc file to specify the default values. Pylint looks for /etc/pylintrc and ~/.pylintrc. The --generate-rcfile option will generate a commented configuration file according to the current configuration on standard output and exit. You can put other options before this one to use them in the configuration, or start with the default values and hand tune the configuration.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Nadia Alramli
  • 111,714
  • 37
  • 173
  • 152
  • 1
    However, if there are others on your team who review your code in ther own environments, they will still get these warnings. – Derek Jun 22 '19 at 06:12
  • 5
    @Derek pylintrc should be versioned with the rest of the code – Aryeh Leib Taurog Sep 14 '20 at 16:34
  • Yes, it should be. At a minimum, that ensures everyone follows the same rules. And PRs containing changes to pylintrc should be treated as deeply suspicious :-) – paxdiablo Mar 31 '22 at 08:46
11

You could try using Python's variable arguments feature:

def myfunction(*args):
    for x in args:
        # Do stuff with specific argument here
hbw
  • 15,560
  • 6
  • 51
  • 58
  • 1
    It's the same as using a list, see below. – Anonymous May 03 '09 at 05:23
  • 1
    Even though, technically, it is correct; it would cause a lot of confusion in the call site. Therefore, I find it to be a bad programming practice. – Can Feb 04 '22 at 12:55
8

Perhaps you could turn some of the arguments into member variables. If you need that much state a class sounds like a good idea to me.

Brian Rasmussen
  • 114,645
  • 34
  • 221
  • 317
  • Doesn't work if I need to refactor a class method and passed variables are local to the big refactored method and are not used in the whole class. – Anonymous May 03 '09 at 05:24
  • 4
    No, but if you extract a new type instead you may be able to turn some of the state into member variables. – Brian Rasmussen May 03 '09 at 05:35
6

I do not like referring to the number. The symbolic name is much more expressive and avoid having to add a comment that could become obsolete over time.

So I'd rather do:

#pylint: disable-msg=too-many-arguments

And I would also recommend to not leave it dangling there: it will stay active until the file ends or it is disabled, whichever comes first.

So better do:

#pylint: disable-msg=too-many-arguments
code_which_would_trigger_the_msg
#pylint: enable-msg=too-many-arguments

I would also recommend enabling/disabling one single warning/error per line.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Igor Stoppa
  • 283
  • 4
  • 12
6

Simplify or break apart the function so that it doesn't require nine arguments (or ignore Pylint, but dodges like the ones you're proposing defeat the purpose of a lint tool).

If it's a temporary measure, disable the warning for the particular function in question using a comment as described in Pylint: Disable-msg for a block or statement?

Later, you can grep for all of the disabled warnings.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Dave
  • 10,369
  • 1
  • 38
  • 35
  • My goal is to break the big function first. Then I could proceed to break the smaller parts further. But I want to avoid this specific pylint warning during the refactoring process, if this is possible. – Anonymous May 03 '09 at 05:20
5

For Python 3, you should just use keyword-only arguments:

File pylint_args_too_many.py

"""Example of a function causing pylint too-many-arguments"""


def get_car(
    manufacturer, model, year=None, registration_number=None, vin=None, color=None
):
    """Returns dict with all required car attributes"""
    return {
        "manufacturer": manufacturer,
        "model": model,
        "year": year,
        "registration_number": registration_number,
        "vin": vin,
        "color": color,
    }


print(repr(get_car(manufacturer="ACME", model="Rocket")))
pylint pylint_args_too_many.py

************* Module pylint_args_too_many
pylint_args_too_many.py:4:0: R0913: Too many arguments (6/5) (too-many-arguments)

------------------------------------------------------------------
Your code has been rated at 6.67/10 (previous run: 6.67/10, +0.00)

File pylint_args.py

"""Show how to avoid too-many-arguments"""


def get_car(
    *, manufacturer, model, year=None, registration_number=None, vin=None, color=None
):
    """Returns dict with all required car attributes"""
    return {
        "manufacturer": manufacturer,
        "model": model,
        "year": year,
        "registration_number": registration_number,
        "vin": vin,
        "color": color,
    }


print(repr(get_car(manufacturer="ACME", model="Rocket")))
pylint pylint_args.py

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)
Tometzky
  • 22,573
  • 5
  • 59
  • 73
2

Use a dataclass:

from dataclasses import dataclass

# declare your data structure
@dataclass
class Struct:
    x1: int
    x2: int
    x3: int
    x4: int
    x5: int

# declare your function operating on this structure
def mysum(s: Struct):
    return s.x1 + s.x2 + s.x3 # ...

def mybigfunction():
    s = Struct(1,2,3,4,5) # instantiate your structure
    x = mysum(s)
    return x

Then mybigfunction() returns 6 (1+2+3).

This is a nice way to group, organize, and document your arguments. It also simplifies type hinting of functions.

Note that all the dataclass decorator does is making your work easier by implementing the __init__ method and others.

Hugo Trentesaux
  • 1,584
  • 1
  • 16
  • 30
0

Python has some nice functional programming tools that are likely to fit your needs well. Check out lambda functions and map. Also, you're using dicts when it seems like you'd be much better served with lists. For the simple example you provided, try this idiom. Note that map would be better and faster but may not fit your needs:

def mysum(d):
   s = 0  
   for x in d:
        s += x
   return s

def mybigfunction():
   d = (x1, x2, x3, x4, x5, x6, x7, x8, x9)
   return mysum(d)

You mentioned having a lot of local variables, but frankly if you're dealing with lists (or tuples), you should use lists and factor out all those local variables in the long run.

easel
  • 3,982
  • 26
  • 28
  • 1
    I can't use a list. In my trivial example my passed variables play the same role. But in a complex scenario the variables have different meanings, so replacing their names (that carry a logical meaning) with list items accessed by index would totally destroy the readability of the code. – Anonymous May 03 '09 at 05:26
  • 2
    You'll have to use the dict then. That said, you're not going to be able to clean stuff up much unless you change some of your requirements. Alternatively you could define a class for all this stuff and then push the logic into various class methods. probably cleaner than a massive if-then tree based on a dict, at least! – easel May 03 '09 at 05:32
-3

I came across the same nagging error, which I realized had something to do with a cool feature PyCharm automatically detects...just add the @staticmethod decorator, and it will automatically remove that error where the method is used.

Peter Mortensen
  • 30,738
  • 21
  • 105
  • 131
Ants
  • 33
  • 1
  • 5
  • 1
    please read this: https://stackoverflow.com/questions/136097/what-is-the-difference-between-staticmethod-and-classmethod – Dolf Andringa Aug 23 '19 at 14:34