Is there any downside to define a function that use one dictionary as an argument instead of several (+10) arguments?

Question

I tried to refactoring someone else's code and I found that there is alot of this pattern

def train(a, b, c, d, e, f, g, h, i, j, k): #Real code has longer name and around 10-20 arguments
    x = Dostuff()
    y = DoMoreStuff(b,c,d,e,f,g,h,x)
    z = DoMoreMoreStuff(a,b,d,e,g,h,j,y)

def DoMoreStuff(b,c,d,e,f,g,h):
    DoPrettyStuff(b,c,d,e,f,g,h) # The actually depth of function call is around 3-4 calls

I though that it would it be better to collect all of that a,b,c,d into one dictionary. I got this idea when I was reading about rail rack's middleware.

env = {"a":1, "b", 2.......}
def train(env):
    x = Dostuff()
    y = DoMoreStuff(env.update({"x":x}))
    z = DoMoreMoreStuff(env.update({"y":y}))

def DoMoreStuff(env)
    DoPrettyStuff(env)

But I am not sure if there is any downside about this approach ? If it does help, every function here has a side-effect.

EDIT: For more clarification. I think most of my problem are about the propagation of variables. The call pattern has very deep and wide hierarchy. Any further suggestion about this one ?

def main(a,b,c,d,e,f,g,h,i,j,k,l,m....z):
    Do(...)
    #maybe other function()

def Do(a,b,c,d,e,f,g,h,i,j,k,l):
    Do1(d,e,f,g,h,i,j,k,l)

def Do1(d,e,f,g,h,i,j,k,l)
    Do2(f,g,h,i,j,k,l)

def Do2(f,g,h,i,j,k,l)
    Do3(f,g,h,i)
    Do4(j,k,l)

EDIT 2: Well, my intention is that I want the code more easier to read and followed. Performance in python code does not matter much since the program spend most of the time on SVM and machine learning stuff. If anyone interest and don't mind a very dirty code that I still working on it. Here is the real code https://github.com/yumyai/TEES/tree/30b3aeaab011e30dcdc3c1151062697861ddcae1

The code start with train.py (training model), classify.py (classify using model). In this case I'll use train.py as an example. https://github.com/yumyai/TEES/blob/30b3aeaab011e30dcdc3c1151062697861ddcae1/train.py: line 136 where it start to call everything under it and pass a gigantic arguments to. These arguments are pass down to https://github.com/yumyai/TEES/blob/30b3aeaab011e30dcdc3c1151062697861ddcae1/Detectors/EventDetector.py: line 49. And then EventDetector pass these argument to it members (line 24-28).

Some argument is reused throughout the code, for example, def train(.......exampleStyles). exampleStyles is passed around to several member that is called by train.

Answer 1

Sending parameters in an array/dictionary/hashmap is critical, as you have to take care that every necessary value is filled. You will have to document the content and the next developer has to read it. With parameters this is done by your compiler.

You should concentrate on getting rid of some parameters in an other way. As you talk about some "core" arguments it seem that you might want to move your methods into a class, or at least some of your parameters should be grouped in a class/some classes.

Sometime your are fetching values from an object and sending multiple parameter instead of sending the object itself.

---

Reading about pipelines might be interesting.

---

Checking your real code this looks more like a configuration object that need to be extracted. You could create this object with a builder to address the issues from my first paragraph.

In addition to that you should try to split you methods into smaller parts.

But first of all you need tests. I didn't found any in your repo?

Answer 2

Well, the answer depends on the meaning of the variables. If they are related (and they should be, since the same functions uses them!), then it's a good idea to group them in a dictionary since it makes the code more readable.

However, even if it makes sense to group all variables into a dict, your env.update() solution is not satisfying because it is unlikely that x and y really belong to env. What have you gained here? Function definitions may be shortened, but you only moved the problem to make it less visible. The next developer will be very surprised to see that you simply replaced function arguments by a dict without thinking about readability. This does not respect the Principle of least astonishment and probably others too.

You should try to group variables that really belong together, make use of *args and **kwargs whenever it makes sense, and otherwise split/reorganize functions that take too much unrelated arguments: they probably break the Single responsibility principe anyway.

Now, be careful when refactoring such functions, make sure you really understand them, and write unit tests or integration tests before changing them to make sure you don't make the code worse than before.

Answer 3

Just a quick note: As far as I see, nobody has mentioned yet that it's a code smell: Long Parameter List. More details are there in Martin Fowler's Refactoring: Improving the Design of Existing Code book.

Chapter 3. Bad Smells in Code, Long Parameter List contains some suggested refactorings:

Use Replace Parameter with Method when you can get the data in one parameter by making a request of an object you already know about. This object might be a field or it might be another parameter. Use Preserve Whole Object to take a bunch of data gleaned from an object and replace it with the object itself. If you have several data items with no logical object, use Introduce Parameter Object.

Answer 4

I though that it would it be better to collect all of that a,b,c,d into one dictionary.

In this particular case, rather than dictionary, I'd say it's probably better to declare a class. In python, objects is a syntax sugar for dictionary anyway.

class A(object):
    def __init__(self, **params):
        assert {'a', 'b'}.issubset(d) # required parameters
        for k,v in dict(c=None, d=4): params.setdefault(k,v)
        self.__dict__.update(params)

    def train(self):
        print self.a, self.b
        self.Dostuff()
        self.DoMoreStuff(self.c)
        self.DoMoreStuff(self.d)
        self.DoMoreMoreStuff()

a = A(a=1, b=2, ...)

of course you don't have to stuff it all in a single class, you might want to split the parameters into multiple classes where it makes sense to do so according to OOP principles.

most functions in the code usually has some "core" argument that keep repeating throughout

that is a good candidate to put them into a single class.

Is there any downside to define a function that use one dictionary as an argument instead of several (+10) arguments?

Yes, you lose parameter checking that is usually done by the interpreter so you'd have to do them yourself.