44
\$\begingroup\$

I have a class with quite a few attributes, most of which are known when I create an instance of the object. So I pass all the values in the constructor:

$op = new OpenIdProvider($imgPath . $name . $ext, 'openid_highlight', 
                         0, 0, 108, 68, 6, $info[0], $info[1], $name);

I'm finding that having this many parameters makes it confusing both when writing and reading the code, as it's not easy to determine which attribute each value corresponds to. Also, this has a bit of a code smell to it - seems like there should be a better way. Any suggestions?

\$\endgroup\$
52
\$\begingroup\$

Martin Fowler's bible book Refactoring does identify a smell called "Long Parameter List" (p78) and proposes the following refactorings:

Of these I think that "Introduce Parameter Object" would best suit:

You'd wrap the attributes up in their own object and pass that to the constructor. You may face the same issue with the new object if you choose to bundle all the values directly into its' constructor, though you could use setters instead of parameters in that object.

To illustrate (sorry, my PHP-fu is weak):

$params = new OpenIDParams();
$params->setSomething( $imgPath . $name . $ext );
$params->setSomethingElse( 'openid_highlight' );

$params->setName( $name );

$op = new OpenIdProvider( $params );

This is a little more verbose but it addresses your concern about not being clear about the attributes' purpose / meaning. Also it'll be a little less painful to add extra attributes into the equation later.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks, very helpful. So is the parameter object recommended over instantiating my object with the default constructor and calling setters directly on the object? \$\endgroup\$ – BenV Jan 22 '11 at 5:01
  • \$\begingroup\$ @BenV you could go either way there. I mulled over both approaches and picked using parameter object because it separates the concern of managing the settings away from the Provider - which has a different responsibility. \$\endgroup\$ – LRE Jan 22 '11 at 5:05
  • \$\begingroup\$ I would go with "Replace Parameter with Method" directly, removing all/most params from the constructor, and adding getters/setters. At least if there is no clear-cut semantic in the set of parameters provided, e.g. OpenIDParams does not seem to add much here, it just moves the issue: "Do I need a constructor with all those parameters for OpenIDParams?"... One more level of abstraction does not always solve the issue :) \$\endgroup\$ – Eric Bréchemier Jan 22 '11 at 8:58
  • 2
    \$\begingroup\$ This works particularly nice in a language with object literals (javascript): new OIDP ({ "arg1" : val1, "arg2": val2 ...}). Similar readability increase works in a language with keyword arguments like python. In both these cases you get something very compact and readable (the keyword strategy doesn't really correspond to a parameter object, but reads close enough that I figured I'd include it). \$\endgroup\$ – kasterma Jan 22 '11 at 16:44
  • 1
    \$\begingroup\$ Before refactoring the constructor specifically, also ask yourself this: could any of these parameters be grouped into something useful? Maybe all these numbers are related and you could create an object from that? That would reduce the number of parameters to the constructor, and generally give you better readability, while being reusable in other places than just that one constructor. \$\endgroup\$ – Sebastian Redl Feb 24 '14 at 16:09
20
\$\begingroup\$

In addition to LRE's answer I would suggest you consider the idea that your class needs that many constructor arguments because it's trying to do too many things.

\$\endgroup\$
8
\$\begingroup\$

As @LRE answer (+1) mentioned his PHP-fu is weak and since his points are correct and valid i just want to provide some more php code to illustrate :

class OpenIdProviderOptions {

    private $path;
    private $name = "default_name";
    private $param1;
    private $optionalParam2 = "foo";

    public function __construct($path, $param1) {
         /* you might take a $options array there for bc */
         /* Also if you have 2-3 REQUIRED parameters and the rest is optional
            i like putting all the required stuff in the constructor so you don't
            have to do any "is that value set" checking here.

            If you have 20 REQUIRED values you might split those into some objects
            or something ;) 
         */
    }

    public function getPath() {
        return $this->path;
    }

    /* etc */


}

class OpenIdProvider {

    private $settings;

    public function __construct(OpenIdProviderOptions $options) {
        $this->settings = $options;
    }

    public function foo {
        $path = $this->settings->getPath();
    }
}

$settings = new OpenIdProviderOptions("myopenid.example.com", "i need this");
$settings->setFoo("bar");
$settings->setBar("fubar");

$myProvider = new OpenIdProvider($settings);
\$\endgroup\$
7
\$\begingroup\$

I think it would be better to put all of your parameters into one array. You then construct the array in a single command when you want to call the constructor.

This approach also has the benefit that it is possible to add more options for the class, see the code below for an example:


class OpenIdProvider
{
    public $_width = 0;
    public $_highliting = '';
    public $_Xposition = 0;
    public $_Yposition = 0;
        ...
        ...

    /**
     * Main constractor to set array of parameters.
     * param array $parameters
     * description  _Xposition , _width , _Yposition ...
     */
    function __constrauct($parameters)
    {
        if(is_array($parameters))
            foreach($parameters as $needle=>$parameter)
                $this->{'_'.$needle} = $parameter;
    }
}
$options = array('path'=>$imgPath . $name . $ext,
                'highliting'=> 'openid_highlight', 
                 'width'=>108, 
                 'height'=>68, 
                 '...'=>6, 
                 '...'=>$info[0], 
                 '...'=>$info[1],
                 '...'=>$name);
$openIdObj = new OpenIdProvider($options);

\$\endgroup\$
  • 4
    \$\begingroup\$ I really dislike this for a number of reasons: No type hinting for classes, no enfored required parameters so you need more error checking for foremost: It's incredible hard to use, the implantation is easy but the api suffers greatly. You'd need to at least repeat all the parameters in the doc block you one even has a chance to create the class without looking at the sourcecode. Still, reading the docs and building an obscure alone is painfull enough for me to dislike that :) Still: Thanks for sharing, just my thoughs \$\endgroup\$ – edorian Jan 24 '11 at 12:24
  • \$\begingroup\$ @edorian can you elaborate? This is how lots of legacy code at my current job works and I'm trying to understand why it's wrong and what's better. Are you basically saying a params array that's just floating around like this is hard to maintain? That objects offer more structure & are more standardized? Can you explain the doc block point? \$\endgroup\$ – Sam Selikoff May 9 '13 at 16:33
  • \$\begingroup\$ Be aware that this class allows setting arbitrary variables inside the OpenIdProvider. $this->{'_'.$needle} = $parameter; this will create any underscore-prefixed variable you like. Even though this is only potentially unsafe, it can also lead to "typo" bugs. As seen in "highliting" in the actual code above! That would silently create $this->_highliting instead of setting $this->_highlighting \$\endgroup\$ – Tomáš Fejfar Mar 29 '17 at 20:20
4
\$\begingroup\$

PHP arrays are powerful. Similar to highelf I would use an array.

However, I would do a number of things differently to highelf.

  1. Don't create public properties from the array you receive (especially using unknown array keys).
  2. Check the values that you receive.

As edorian said, there is no type hinting, but you can enforce the required parameters manually. The advantages of this approach are:

  1. Its easy to remember the number of parameters (1).
  2. The order of the parameters is no longer important. (No need to worry about keeping a consistent order of parameters, although alphabetically works well.)
  3. The calling code is more readable as the parameters are named.
  4. It is very hard to accidentally set the width to the height even though the type is the same. (This is easily overlooked by type hinting).

So here is what I would do:

class OpenIdProvider
{
   protected $path;
   protected $setup;

   public function __construct(Array $setup)
   {
      // Ensure the setup array has keys ready for checking the parameters.
      // I decided to default some parameters (height, highlighting, something).
      $this->setup = array_merge(
         array('height'       => 68,
               'highlighting' => 'openid_highlight',
               'info'         => NULL,
               'name'         => NULL,
               'path'         => NULL, 
               'something'    => 6, 
               'width'        => NULL),
         $setup);

      // The following checks may look lengthy, but it avoids the creation of
      // the OpenIdProviderOptions class that seems to do very little. Also,
      // these would appear in that class to check the constructor values it
      // received properly.
      if (!is_array($this->setup['info']))
      {
         throw new InvalidArgumentException(
            __METHOD__ . ' requires info as array');
      }

      if (!is_string($this->setup['name']))
      {
         throw new InvalidArgumentException(
            __METHOD__ . ' requires name as string');
      }

      if (!is_string($this->setup['path']))
      {
         throw new InvalidArgumentException(
            __METHOD__ . ' requires path as string');
      }

      if (!is_int($this->setup['width']))
      {
         throw new InvalidArgumentException(
            __METHOD__ . ' requires width as int');
      }

      // If you use a setup parameter a lot you might want to refer to it from
      // this, rather than having to go via setup.
      $this->path =& $this->setup['path'];
   }

   public function internalFunction()
   {
      // Use the parameters like so.
      echo $this->setup['width'];
      echo $this->path;
   }
}

// The call is very easy.
$op = new OpenIdProvider(
   array('info'  => $info,
         'name'  => $name,
         'path'  => $imgPath . $name . $ext,
         'width' => 108));
\$\endgroup\$
3
\$\begingroup\$

Had same problem. Here's my solution:

  1. Try as hard as possible to break up your class into various pieces.
  2. Have these pieces passed as parameters.

For example it seems your object draws it's own image. Delegate that to another class (IOpenIDImage). If your provider is supposed to open up a frame or tab to do its work then have another class handle that IOpenIDFrame.

Now your code becomes:

$Image    = new OPenIDImage('google', 100, 150);
$Frame    = new OPenIDFrame(......);
$Data     = new OPenIDSettings('host', 'name', 'auth-key');
$Provider = new OpenIDProvider(Image $Image, Frame $Frame, Settings $Data);

This makes your code:

  1. Smaller / Compact. More easy to understand.
  2. Easier to debug. Since Everything is in a separate area.
  3. Easier to test. You can test different aspects like image.
  4. Easier to modify. You dont have to modify the entire class.

I follow the following rules:

  • 2 Functions are better than 1
  • 2 Classes are better than 1
  • 2 Variables are better than 1
\$\endgroup\$
1
\$\begingroup\$

You may use builder pattern here. I strongly recommend introduce fluent interface within it.

Here's stackoverflow link

The builder pattern is a good choice when designing classes whose constructors or static factories would have more than a handful of parameters.

At the end you will have something like

$op = new OpenIdProviderBuilder()
              .setImagePath($imgPath . $name . $ext)
              .setOpenId('openid_highlight')
              .setX(0)
              .setY(0)
              .setR(100)
              .setG(80)
              .setB(120)
              .setInfo("some info");
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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