Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
add comment

5 Answers

up vote 35 down vote accepted

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

.. snip ..

$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.

HTH

share|improve this answer
1  
Thanks, very helpful. So is the parameter object recommended over instantiating my object with the default constructor and calling setters directly on the object? –  BenV Jan 22 '11 at 5:01
 
@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. –  LRE Jan 22 '11 at 5:05
 
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 :) –  Eric Bréchemier Jan 22 '11 at 8:58
2  
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). –  kasterma Jan 22 '11 at 16:44
add comment

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.

share|improve this answer
add comment

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);
share|improve this answer
add comment

I think it better to be all your parameter in one array and you must make good command for using this at other time It has some benefit and its that u can develop and add more option for this class. like this


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

share|improve this answer
4  
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 –  edorian Jan 24 '11 at 12:24
 
@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? –  Sam Selikoff May 9 '13 at 16:33
add comment

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));
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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