Instantiating objects with many attributes

Question

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?

Answer 1

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

• Replace Parameter with Method (p292)
• Introduce Parameter Object (295)
• Preserve Whole Object (298)

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.

Answer 2

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.

Answer 3

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);

Answer 4

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);

Answer 5

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));

Answer 6

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

Answer 7

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");