Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
5  
If you asked me, if you have a function that requires that many independent parameters, it's trying to do too much. You could also consider changing it so the function uses keyword arguments, than way you can switch between the two forms easily. –  Jeff Mercado Feb 20 '13 at 7:31
    
Thanks for the suggestion. I was planning to break down function into bite-side too. –  Tg. Feb 20 '13 at 7:43
2  
I really would like to see your real methods signatures. I'm pretty sure we could help you in more detail if we know what your are going to do. –  mnhg Feb 20 '13 at 10:21
    
@mnhg I want to make code more easier to follow and easy to read. This code is actually a NLP analysis pipeline that I fork from and several parameters/arguments are either 1. Doing to many thing at one method as others suggest 2. Keep everything in sync, because it rely alot on external file/arguments. Each function is bloat with argument mostly because of this. I'll EDIT my question to post a my code then. –  Tg. Feb 20 '13 at 10:54
    
Pipeline is the keyword. If you follow a data-driven approach you should easily be able to pack your data in a kind of a work package which you can pipe throw your line. Has this to be parallelisable? –  mnhg Feb 20 '13 at 11:00

4 Answers 4

up vote 3 down vote accepted

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?

share|improve this answer
    
You got me there about the test. I "forgot" to do that... I should did it from the start, shouldn't I. –  Tg. Feb 20 '13 at 15:43
    
Yes :) "Test first" should be your target. –  mnhg Feb 21 '13 at 5:58

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.

share|improve this answer
    
From my understanding. You suggest that updating env should be avoid. How about I use DoMoreStuff(env, x) instead ? That should do it since most functions in the code usually has some "core" argument that keep repeating throughout. For about grouping some similar argument together, sadly, it is already grouped and still gigantic. You also hit a nail in the head about breaking a "single responsibility principle", that was what I called a "side-effect". And yes, I am about to refactor that out too. –  Tg. Feb 20 '13 at 7:39
    
Yeah, DoMoreStuff(env, x) should be OK if it makes sense. –  Quentin Pradet Feb 20 '13 at 8:07

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.

share|improve this answer
    
Actually I did, but not as nice as you :) –  mnhg Feb 21 '13 at 6:00

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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