42

I am reading "Clean Code" and having trouble figuring out how to keep some of my functions (usually constructors) to their MAXIMUM of 3 parameters.

Often my objects need an awful lot of information to work - am I supposed to make a small constructor and then use mutator functions to give them all of the information? This doesn't seem any better than just using a big constructor.

As an example, I have a "MovablePatch" class. It lets the user drag a square around in a window. It needs a several parameters, including Radius, Color, Renderer, InitialPosition, and Visibility. Currently I collect all of these from my GUI and then call:

MovablePatch(int radius, Renderer* renderer, Color color,  Position initial, bool visibility)

These are only some of the things that I need in this class. Can anyone suggest how else I might package this information to pass to the constructor? I don't see any obvious "break it into smaller classes" appearing here.

3
  • 9
    Look up the builder pattern.
    – Bashwork
    Dec 29, 2011 at 19:07
  • 1
    Do you have to follow that guide line? Otherwise I would pretty much say ignore it where it's not obvious how to apply it. Imo if modifications to follow a guideline aren't intuitive it's generally not worth it to refactor the code to ensure the guideline is followed no matter what. For unintuitve solution that can easily lead to more complicated (and therefore bugprone) code which doesn't even have better readability (an example for this is the mentioned small constructor followed by mutator function calls or the builder pattern, which both make it easy to forget to set some value).
    – Grizzly
    Dec 29, 2011 at 20:03
  • 6
    Using a simple struct for related attributes to be passed as one parameter has been done since before I was born I'm pretty sure.
    – AJG85
    Dec 29, 2011 at 20:54

5 Answers 5

31

You could have

MovablePatch(Renderer* renderer, CircleAppearance circleAppearance)

where CircleAppearance gathers the other info.

However, clean code and other books that generalize about what good code should look like, are aiming for 80 percent of the code out there. Your code seems to be "closer to the metal" than the typical LoB (Line of Business) variety. As such, you may run into places where certain coding ideals are not applicable.

The most important part is that you're thinking about it and trying to keep things nice and tidy! :)

31

Do not take maxims like "thou shalt not have more than 3 parameters in thy constructors" at face value. If you have the slightest chance of making an object immutable, make it; and if it being immutable means that it is going to have a constructor with 50 parameters, so be it; go for it; don't even think about it twice.

Even if the object is going to be mutable, still, you should pass its constructor as many parameters as necessary so that immediately upon construction it will be in a valid and meaningful state. In my book, it is absolutely impermissible to have to know which are the magic mutator methods that have to be called (sometimes even in the right order) before any other methods can be invoked, under penalty of segfault.

That having been said, if you would really like to reduce the number of parameters to a constructor, or to any function, simply pass this method an interface that it can invoke to get from it the stuff it needs in order to work.

1
  • 8
    +1 for this. Immutability is far more beneficial than small parameter lists.
    – vocaro
    Dec 30, 2011 at 3:05
21

Some of the things you are passing in could be abstracted into a larger construct. For example, visibility, color, and radius, could make sense to be placed into an object that you define. Then, an instance of this class, call it ColoredCircle, could be passed into the constructor of MovablePatch. A ColoredCircle doesn't care where it is or what renderer it is using, but a MovablePatch does.

My main point, is that from an OO perspective, radius isn't really an integer, it's a radius. You want to avoid these long constructor lists because it is daunting to understand the context of these things. If you collect them into a larger class, kind of like how you already have with Color and Position, you can have fewer parameters passed in and make it easier to understand.

5
  • 2
    Doesn't this only shift the problem? You also have to initialize the ColoredCircle class somehow, and there are still four parameters left ... Dec 29, 2011 at 19:14
  • 2
    @MartinStettner, yes you have to initialize the ColoredCircle first, but that should be four of the parameters. That means the MovablePatch would only take a ColoredCircle and a Renderer*. Dec 29, 2011 at 19:23
  • 1
    Should Position should be stored in the Patch or in the Circle? I think the patch has the position, not the circle. Dec 29, 2011 at 19:32
  • 1
    This at least separates the renderer from the characteristics of the circle. That's good. You can organize circle attributes later if need be. Dec 29, 2011 at 19:35
  • 3
    Also remember that by having ColoredCircle as a separate struct, it can have its own constructor that provides default values, so if the programmer doesn't actually want to specify everything they don't have to without it becoming a big mess on the ctor.
    – fluffy
    Dec 30, 2011 at 2:25
12

The Named Parameter Idiom is useful here. In your case, you might have

class PatchBuilder
{
public:
    PatchBuilder() { }
    PatchBuilder& radius(int r) { _radius = r; return *this; }
    PatchBuilder& renderer(Renderer* r) { _renderer = r; return *this; }
    PatchBuilder& color(const Color& c) { _color = c; return *this; }
    PatchBuilder& initial(const Position& p) { _position = p; return *this; }
    PatchBuilder& visibility(bool v) { _visibility = v; return *this; }

private:
    friend class MovablePatch;
    int _radius;
    Renderer* _renderer;
    Color _color;
    Position _position;
    bool _visibility;
};

class MovablePatch
{
public:
    MovablePatch( const PatchBuilder& b ) :
        _radius( b._radius );
        _renderer( b._renderer );
        _color( b._color );
        _position( b._position );
        _visibility( b._visibility );
    {

    }

private:
    int _radius;
    Renderer* _renderer;
    Color _color;
    Position _position;
    bool _visibility;
};

then you use it like so

int
main()
{
    MovablePatch foo = PatchBuilder().
        radius( 1.3 ).
        renderer( asdf ).
        color( asdf ).
        position( asdf ).
        visibility( true )
     ;
}

overly simplified, but I think it gets the point across. If certain parameters are required they can be included in the PatchBuilder constructor:

class PatchBuilder
{
public:
    PatchBuilder(const Foo& required) : _foo(required) { }
    ...
};

Obviously this pattern degenerates into the original problem if all arguments are required, in which case the named parameter idiom isn't applicable. The point being, this isn't a one size fits all solution, and as Adam describes in the comment below there are additional costs and some overhead with doing so.

5
  • 1
    I took the liberty to add the return *this statements to the PatchBuilder's methods ;) ... Dec 29, 2011 at 19:21
  • 11
    This new fluent builder fixation is dangerous. You need to have a runtime check to see if everything was populated. I would not recommend this. Unnecessary complexity that's going to cost to maintain. Dec 29, 2011 at 19:33
  • 1
    @AdamDymitruk I have updated the answer with some wording based on your suggestion.
    – Sam Miller
    Dec 29, 2011 at 19:49
  • 3
    I have to agree with Adam Dymitruk. Not only is there no way to ensure at compile time that all values were initialized (and the necessary copying and runtime checking will probably incure some overhead), but it is also not self documenting in the way that it doesn't make clear which values need to be set. If every value is a different function argument a look at the function definition will tell me what I have to pass. For a builder one has to look at the definitions of all methods (might still not be enough, since arguments might be optional=>look at documentation), making it harder to use.
    – Grizzly
    Dec 29, 2011 at 19:55
  • 3
    @Grizzly: There is a way to ensure at compile time that all values were initialised. It’s just that C++ isn’t the most concise or readable when it comes to expressive types, so it’s almost certainly not worth it.
    – Jon Purdy
    Dec 30, 2011 at 1:17
8

One good option is to use a Builder pattern, where each "setter" method returns the own instance, and you can chain the methods as you need.

In your case, you will get a new MovablePatchBuilder class.

The approach is very useful and you can find it in many different frameworks and languages.

Refer here to see some examples.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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