Is passing a lot of parameters to parent::__construct a code smell/poor OOP?

Question

I was writing some code and I began to feel a little uncomfortable with messy parent::__construct calls and I was wondering firstly is it bad OOP practice and secondly is there a cleaner way to do it? See the particularly extreme example below that triggered my question.

<?php
class BrowseNodeLookupRequest extends Request {

    protected $BrowseNodeId;

    public function __construct($Service, $AWSAccessKeyID, $AssociateTag,
            $Operation, $MerchantID = null, $ResponseGroup = null,
            $Version = null, $Style = null, $ContentType = null,
            $XMLEscaping = null, $Validate = null, $BrowseNodeId) {
        parent::__construct($Service, $AWSAccessKeyID, $AssociateTag,
                $Operation, $MerchantID, $ResponseGroup, $Version, $Style,
                $ContentType, $XMLEscaping);
        $this->setBrowseNodeId($BrowseNodeId);
    }

    protected function setBrowseNodeId($BrowseNodeId) {
        if (is_string($BrowseNodeId)) {
            $this->BrowseNodeId = $BrowseNodeId;
        } else {
            throw new Exception('BrowseNodeLookupRequest Parameter (BrowseNodeId
                                 ) Must be a String');
        }
    }

}
?>

I don't think it's bad. The whole point of extending a class is to add your own functionality. If you re-implement the parent (or call it internally to the class) you are defeating the purpose of extending it. — Cfreak, Dec 12 '11 at 18:38
For those interested in how-to handle large number of arguments http://stackoverflow.com/questions/2112913/should-my-php-functions-accept-an-array-of-arguments-or-should-i-explicitly-requ — Mike B, Dec 12 '11 at 19:01
Your function takes too many arguments. You also provide defaults for most of them, but then tack on a argument with no default. You need to provide a default for all arguments (after the first default) or none of them, you can't omit the default for the last value. — user229044, Dec 12 '11 at 19:12
Yes good point about the the final argument. I guess the options I have in front of me are to either use an array to pass the arguments or try to contain the various arguments into objects. I naturally dislike the first option simply due to code hinting/readability issues but the second option seems to make little sense from an OOP point of view as all of these variables ARE specifically related to the object they currently reside in and do not really form any natural sub-groupings or other objects. Any further tips? [Good link from Mike B] — ShaunUK, Dec 12 '11 at 19:23

score 3 · Answer 1 · answered Dec 12 '11 at 19:20

3

It's bad practice to have that many arguments to any function, whether it be to __parent::construct or not.

It's just too easy to mess up, particularly in PHP. A lot of times this is an indication that you are missing objects (or have too tight of coupling between parts). I would even prefer passing a "configuration" object if you cannot come up with any other missing objects.

class ConfigFoo
{
  public $Service, $AWSAccessKeyID, ..., $foo, $bar;
}

$cfg = new ConfigFoo();
$cfg->Service = 'whatever';
...

$req = new BrowseNodeLookupRequest($cfg);

This is basically a more structured way to pass an array of parameters. The configuration object can extend other configuration objects to follow your other objects.

And of course the class can be more advanced than simple public properties. You can manage data integrity, and so forth.

To be clear: I wouldn't resort to the above unless a) there are no other missing intermediate objects and b) there are a sufficient number of parameters that makes using a simple array just as problematic as a long list of function arguments.

answered Dec 12 '11 at 19:20

Matthew

47,584
11
86
98

I agree that coming up with a different class would be more elegant but I can't really think of one that doesn't just make the code more unclear. Those are all the common attributes of a Request as specified in the API. One of the goals I set myself when writing this was that it be easy to understand to anyone who is familiar with the docs for the API. Isn't the Config Class just moving the issue rather than addressing it? – ShaunUK Dec 12 '11 at 19:35
I should point out that I'm an inexperienced programmer and I'm not trying to say anyone is wrong and I'm right. I'm just really keen to get to the bottom of this so I can write better code in future. So all my responses are meant with respect. – ShaunUK Dec 12 '11 at 19:39
@user1091431, Using a config class has benefits: 1) Any of the properties can be given default values by the `ConfigFoo` class. This happens in one place, even if that set of parameters is used by many functions. 2) Order doesn't matter. 3) The names must be explicit by the calling code. 4) The config class can validate the data before it gets sent to the function. All of these things mean that future changes can easily be applied; nothing will feel "tacked on." – Matthew Dec 12 '11 at 19:46
Finally, you can do away with the config class entirely if it is truly part of the master object. Simply: `$req = new BrowseNodeLookupRequest(); $req->Service = 'whatever'; ...` If you can figure out which few parameters are actually always needed, then make _only_ those part of the constructor. – Matthew Dec 12 '11 at 19:48
Thank for the helpful answers Matt. Your last comment sounds like a good idea. So I could move non-required parameters (the ones I have set as "=null") out of the constructor and make them either public or perhaps better yet expose a ->SetService() method for them? – ShaunUK Dec 12 '11 at 19:55
@ShaunUK, Yes, that is probably the simplest way to clean up the code. There are many equally valid ways to handle the problem as I and others have mentioned. Which is best will be something you'll need to decide depending on what type of objects you are dealing with. – Matthew Dec 12 '11 at 20:52

score 1 · Answer 2 · answered Dec 12 '11 at 18:35

1

Waaaay too many params IMO. Id either pass in an array or potentially separate concerns into various models, and then inject those into the object via the constructor.

answered Dec 12 '11 at 18:35

prodigitalson

60,050
10
100
114

correct me if i am wrong he was refferring to some thing about parent constructor call. I mean he thinks it i a little confusing between __construct() and ::__construct() calls – noobie-php Dec 12 '11 at 18:38
I guess I'm asking this because every time I extend a class in this fashion its going to involve repeating the code of the parent:construct() call. I was wondering if there was a cleaner way to require the parent class parameters and pass them to the parent constructor without having to write it out each time. – ShaunUK Dec 12 '11 at 18:43
1

@user1091431 There might be something you could rig up with `func_get_args` and `call_user_func_array`, but you shouldn't. What your doing is the "correct" way to pass arguments from a child-class constructor to a parent-class constructor. You simply need to reduce the number of arguments, no function (constructor or otherwise) should have that many arguments. – user229044 Dec 12 '11 at 19:09

score 0 · Answer 3 · answered Dec 12 '11 at 18:39

0

It's absolutely not bad to call parent::__construct();

In php this may not always be necessary since if a parent has own constructor and sub-class does not define a constructor, the parent's constructor is automatically called.

In contrast in Java there is no such luxury and you always have to call parent constructor in Java, so this is something you see all the time in Java and probably in other languages that allow overloading constructor methods.

answered Dec 12 '11 at 18:39

Dmitri Snytkine

1,096
1
8
14

He's asking specifically if it's bad practice to involve so many arguments, not whether or not it should be called at all. – user229044 Dec 12 '11 at 19:06
I did not see any question about too many arguments. The question asks about calling parent::__construct(), which is not bad at all. – Dmitri Snytkine Dec 12 '11 at 19:11
the `$load, $of, $arguments` in the title was a clue, but it's pretty obvious from the question body that he's interested in *cleaning up* the process of passing arguments to the parent, not *removing* them entirely. – user229044 Dec 12 '11 at 19:14
Sorry I should have tied what I wrote to the title better but meager is correct the question is "Is passing a lot of parameters to parent::__construct a code smell/poor OOP?" – ShaunUK Dec 12 '11 at 19:16
1

Yes, too many parameters is not a good practice but there are situations when it is necessary. – Dmitri Snytkine Dec 12 '11 at 19:27

score 0 · Accepted Answer · answered Dec 13 '11 at 16:40

With thanks to all the commenters, particularly Matthew, the solution I've gone with is to remove optional parameters from the constructor method of the parent class and add them to public setter methods available in the parent class. Revised code is below with usage example. Note that the '->setValidate' and '->setResponseGroup' are new methods inherited from the parent class. Obviously if you are in a similar situation but all of your parameters are required then you need to go for one of the other options outlined in the responses.

Revised Class:

<?php
class BrowseNodeLookupRequest extends Request {

    protected $BrowseNodeId;

    public function __construct($Service, $AWSAccessKeyID, $AssociateTag,
            $Operation, $BrowseNodeId) {
        parent::__construct($Service, $AWSAccessKeyID, $AssociateTag, $Operation);
        $this->setBrowseNodeId($BrowseNodeId);
    }

    protected function setBrowseNodeId($BrowseNodeId) {
        if (is_string($BrowseNodeId)) {
            $this->BrowseNodeId = $BrowseNodeId;
        } else {
            throw new Exception('BrowseNodeLookupRequest Parameter (BrowseNodeId
                                 ) Must be a String');
        }
    }

}
?>

Usage Example:

<?php
$noderequest = new BrowseNodeLookupRequest($Service, $AWSAccessKeyID, $AssociateTag,
            $Operation, $BrowseNodeId);
//If the optional params are required//
$noderequest->setResponseGroup('Blah');
$noderequest->setValidate('True');
//etc.
?>

Is passing a lot of parameters to parent::__construct a code smell/poor OOP?

4 Answers4