Avoid excessive function parameters: class-centered or function-centered approach?

Question

How would you fix the following bad code that passes too many parameters around?

void helper1(int p1, int p3, int p5, int p7, int p9, int p10) {
  // ...
}

void helper2(int p1, int p2, int p3, int p5, int p6, int p7, int p9, int p10) {
  // ...
}

void foo(int p1, int p2, int p3, int p4, int p5, int p6, int p7, int p8, 
         int p9, int p10) {
  helper1(p1, p3, p5, p7, p9, p10);
  helper2(p1, p2, p3, p5, p6, p7, p9, p10);
}

I see two different approaches:

Approach 1: Put all functions in a class

class Foo {
  private:
    int p1, p2, p3, p4, p5, p6, p7, p8, p9, p10;
    void helper1() {}
    void helper2() {}
  public:
    void foo() {
      helper1();
      helper2();
    }
    // add constructor
};

Approach 2: Just pass parameters as a class

struct FooOptions {
    int p1, p2, p3, p4, p5, p6, p7, p8, p9, p10;
};

void helper1(const FooOptions& opt) {
  // ...
}

void helper2(const FooOptions& opt) {
  // ...
}

void foo(const FooOptions& opt) {
  helper1(opt);
  helper2(opt);
}

What are the advantages and disadvantages of the approaches?

An advantage of Approach 1 is that -- if you make the helper functions virtual -- you can subclass and overload them, adding flexibility. But then, in my case (outside of the toy mini example that I gave) such helpers are often templated, so they cannot be virtual anyway.

An advantage of Approach 2 is that the helper functions can easily be called from other functions, too.

(This question is related, but does not discuss these two alternatives.)

Answer 1

If p1,p2,etc... are really just options (flags, etc...), and are completely unrelated to each other, then I would go with the second option.

If some of them tend to be always together, then maybe there are some classes waiting to appear here, and I would go with the first option, but probably not in one big class (you probably actually have several classes to create -- it's hard to say without the names of the actual parameters).

class Something {
   int p1, p3, p5 ...;
   void helper1a();
};

class SomethingElse {
   int p7, p9, p10 ...;
   void helper1b();
};

class Options {
   int p2, p4, ...
};

void foo(Something &s, SomethingElse & else, Options &options) {
  helper2(options);
  s.helper1a();
  else.helper1b();
}

Answer 2

The approach of putting them into a class does not "feel" correct. It seems like an attempt to retrofit old code into looking like something that it is not. Over time, it might be possible to move toward an "object-oriented" style of code, but it seems more likely it would end up being a bad mix of two paradigms. But that is mainly based on my thoughts of what would happen with some non-OO code that I work with.

So between those two options, I would think the struct option is a little better. It leaves the flexibility open for another function to be able to package up the parameters into a structure and call one of the helper functions. But this method to me seems like it has a problem with the self-documenting aspect. If it becomes necessary to add a call to Helper1 from some other location, it is not as clear which parameters must be passed to it.

So while it feels like a number of down votes coming my way, I think the best way is to not change it. The fact that a good editor will show you the parameter list when you type in a new call to one of the functions makes it pretty simple to get it right. And the cost unless it is in a high traffic area is not all that great. And with 64-bit environments it is even less because more of the parameters (is it 4?) are typically sent in registers.

Here is a related question talking about this with lots of responders weighing in.

Answer 3

I rewrote the classes and structs so they could be analyzed as follows:

class Point {
      private:
        int x, y
        void moveUp() {}
        void moveRight() {}
      public:
        void moveUpAndRight() {
          moveUp();
          moveRight();
        }
        // add constructor
    };

struct Point {
  int x, y;
};

void moveUp {}
void moveRight {}
void moveUpAndRight {
  moveUp {}
  moveRight {}
}

Using classes allows you to act upon your object. Code that uses the point object will use a standard interface to move the point around. If the coordinate system changes and the point class is modified to detect the coordinate system it is in, this will allow the same interface to be used in multiple coordinate systems (not breaking anything). In this example it makes sense to go with approach 1.

If your just grouping related data to be accessed for organization purposes...

struct Dwarves {
  int Doc, Grumpy, Happy, Sleepy, Bashful, Sneezy, Dopey;
}

void killDwarves(Dwarves& dwarves) {}
void buryDwarves(Dwarves& dwarves) {}

void disposeOfDwarves(Dwarves& dwarves) {
  killDwarves(dwarves);
  buryDwarves(dwarves);
}

Then it makes sense to go with Approach 2.

This is an OO design choice so there is multiple correct solutions and from what we're given it's difficult to give a definitive answer.

Answer 4

Short Answer:

Happy New Year! I'd avoid option #1 and only go with option #2 if the parameters can be separated into clear and logical groups that make sense away from your function.

Long Answer

I have seen many examples of functions as you described from coworkers. I'll agree with you on the fact that it's a bad code smell. However, grouping parameters into a class just so you don't have to pass parameters and deciding rather arbitrarily to group them based on those helper functions can lead to more bad smells. You have to ask yourself if you're improving readability and understanding for other that come after you.

calcTime(int p1, int p2, int p3, int p4, int p5, int p6) {
    dist = calcDistance( p1, p2, p3 );
    speed = calcSpeed( p4, p5, p6 );
    return speed == 0 : 0 ? dist/speed; }

There you can group things to be more understandable because there is a clear distinction amongst parameters. Then I would suggest approach #2.

On the other hand, code in which I've been handed often looks like:

calcTime(int p1, int p2, int p3, int p4, int p5, int p6) {
    height = height( p1, p2, p3, p6 );
    type = getType( p1, p4, p5, p6 );
    if( type == 4 ) {
        return 2.345; //some magic number
    }
    value = calcValue( p2, p3, type ); //what a nice variable name...
    a = resetA( p3, height, value );
    return a * value; }

which leaves you with a feeling that these parameters aren't exactly friendly to breaking up into something meaningful class-wise. Instead you'd be better off served flipping things around such as

calcTime(Type type, int height, int value, int p2, int p3)

and then calling it

calcTime(
    getType( p1, p4, p5, p6 ),
    height( p1, p2, p3, p6 ),
    p3,
    p4 );

which may send shivers up your spine as that little voice inside your head screams "DRY, DRY, DRY!" Which one is more readable and thus maintainable?

Option #1 is a no-go in my head as there is a very good possibility someone will forget to set one of the parameters. This could very easily lead to a hard to detect bug that passes simple unit tests. YMMV.

Answer 5

Option #2.

However, if you think hard about the domain, you will probably find that there is a meaningful grouping of the parameter values and they can be mapped to some entity in your domain.

Then, you can create instances of this object, pass it in to your program.

Typically this might something like an Application object, or a Session object, etc.

Answer 6

I don't know. It really depends on what the parameters are. There's no magical "make 20 function parameters disappear" design pattern you can invoke. The best you can do is refactor your code. Depending on what the parameters represent, it might be meaningful to group some of them into a parameter class, or put everything into one monolithic class, or split the whole thing out into multiple classes. One option that may work is some form of currying, passing the objects a bit at a time, each time yielding a new object on which the next call can be invoked, passing the next batch of parameters. This especially makes sense if some parameters often stay the same across calls.

A simpler variant of this could be to wrap everything in a class, where some parameters are passed in the constructor (the ones that typically stay the same across a whole batch of calls), and others, the ones that must be supplied per-call as they're constantly changing, are passed in the actual function call (probably an overloaded operator()).

Without knowing the actual meaning of your code, it's impossible to say how it can be best refactored.

Answer 7

Don't use classes as pure data storage and don't use fields to pass data to methods. Usually if a method requires too many parameters it does too much, so you should extract functionality from it into separate methods. Maybe you could provide a more concrete example of your methods so we can discuss it from a bigger perspective.