The 13 parameter function

PHP programming forum. Ask questions or help people concerning PHP code. Don't understand a function? Need help implementing a class? Don't understand a class? Here is where to ask. Remember to do your homework!

Moderator: General Moderators

Post Reply
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

The 13 parameter function

Post by Ollie Saunders »

Just wanted to share that with you all.
What do you think? :P

Code: Select all

/**
 * Create a new site with a video (yes there are 13 parameters)
 *
 * @param string $title
 * @param int $clientId
 * @param int|null $subclientId
 * @param bool $useQuestionnaire
 * @param string|null $feedbackRequest
 * @param string|null $stream
 * @param string|null $download
 * @param string|null $questionnaireStart
 * @param string|null $questionnaireEnd
 * @param string|null $hqVideoUrl
 * @param string|null $lqVideoUrl
 * @param int $widthDim
 * @param int $heightDim
 *
 * @return int the insert_id of the newly created site
 */
public function _createSiteWithVideo($title, $clientId, $subclientId, $useQuestionnaire, $feedbackRequest,
                                      $stream, $download, $questionnaireStart, $questionnaireEnd,
                                      $hqVideoUrl, $lqVideoUrl, $widthDim, $heightDim)
{
    $this->begin();
	if ($feedbackRequest === null and $stream === null and $download === null
	    and $questionnaireStart === null and $questionnaireEnd === null) {

	    $msgSetId = 1; // all null is already reserved under 1
        } else {
    	    $q =
    	    "INSERT INTO MsgSet (feedbackRequest, stream, download,
                                 questionnaireStart, questionnaireEnd)
             VALUES ('$feedbackRequest', '$stream', '$download',
                     '$quesionnaireStart', '$questionnaireEnd')";
    	    $this->exec($q, __FUNCTION__, true);
    	    $msgSetId = $this->_db->insert_id;
        }

        $useQuestionnaire = (int)$useQuestionnaire;
	    $q =
	    "INSERT INTO Site (title, _clientId, _subClientId, _msgSetId,
                           useQuestionnaire, useVideo, hqVideoUrl,
                           lqVideoUrl, widthDim, heightDim)
             VALUES ('$title', $clientId, $subclientId, $msgSetId,
                     $useQuestionnaire, 1, $widthDim, $heightDim)";
	    $this->exec($q, __FUNCTION__, true);
	    $siteId = $this->_db->insert_id;
    $this->commit();

    return $siteId;
}
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

Erm... is this a plugin or something?
User avatar
MrPotatoes
Forum Regular
Posts: 617
Joined: Wed May 24, 2006 6:42 am

Post by MrPotatoes »

how about you take in a string array where you have those 7 strings passed in? that way you don't have a retardedly long parameter list.

you can prolly get it down to 3 parameters if you use 3 arrays. just a thought
User avatar
Chris Corbyn
Breakbeat Nuttzer
Posts: 13098
Joined: Wed Mar 24, 2004 7:57 am
Location: Melbourne, Australia

Post by Chris Corbyn »

Dude, you should pass an associative array for functions like that 8O .... how the heck could anyone remember the order 13 parameters appear in? ;)
User avatar
volka
DevNet Evangelist
Posts: 8391
Joined: Tue May 07, 2002 9:48 am
Location: Berlin, ger

Post by volka »

And have them grouped in some way.
As associative array or maybe even as object e.g. a video object, providing the hq/lq url and dimensions.
User avatar
sweatje
Forum Contributor
Posts: 277
Joined: Wed Jun 29, 2005 10:04 pm
Location: Iowa, USA

Post by sweatje »

Nearly every reference for refactoring will include a "code smell list", of which "long parameter list" nearly always appears.

http://wiki.java.net/bin/view/People/Sm ... factorings

Long Parameter List
Don't pass in everything the method needs; pass in enough so that the method can get to everything it needs.
possible solutions:
Replace Parameter with Method
Preserve Whole Object
Introduce Parameter Object

see also http://c2.com/cgi/wiki?TooManyParameters
alex.barylski
DevNet Evangelist
Posts: 6267
Joined: Tue Dec 21, 2004 5:00 pm
Location: Winnipeg

Post by alex.barylski »

It's not *that* bad

Windows CreateWindow() has 11 arguments...and I'm willing to bet there are others which are even worse...

Likley Shell extension functions, etc...

Some functions have enormous argument lists and to boot they also have a structure which in itself has another 10-15 fields required to be set...
timvw
DevNet Master
Posts: 4897
Joined: Mon Jan 19, 2004 11:11 pm
Location: Leuven, Belgium

Post by timvw »

Apart from the method signature i wonder where you prepare your data for use in a SQL query... To me it appears you're using it unprepared... (Which i find a criminal act :p)
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Erm... is this a plugin or something?
nope
how about you take in a string array where you have those 7 strings passed in? that way you don't have a retardedly long parameter list.
you can prolly get it down to 3 parameters if you use 3 arrays. just a thought
when you use parameter list an IDE will prompt you for the right data in the right order and you can't make mistakes. If you use assoc arrays you can easily typo any of the array keys. Really annoying bugs.
Dude, you should pass an associative array for functions like that Shocked .... how the heck could anyone remember the order 13 parameters appear in? Wink
See point above, this function is also likely to be used only once.
Nearly every reference for refactoring will include a "code smell list", of which "long parameter list" nearly always appears.

http://wiki.java.net/bin/view/People/Sm ... factorings
Long Parameter List Don't pass in everything the method needs; pass in enough so that the method can get to everything it needs.
This makes a very good point actually. I can reduce this to a single parameter :P
I'm going to go and do that now :)
Apart from the method signature i wonder where you prepare your data for use in a SQL query... To me it appears you're using it unprepared... (Which i find a criminal act :p)
Nope, but then you weren't to know because my code is so <span style='color:blue' title='I&#39;m naughty, are you naughty?'>smurf</span> clever.

Code: Select all

/**
	 * This method overloading function is used to preprocess all input to a
	 * query with escape string.
	 *
	 * The user of the class will be able to call a private function such _getSetting()
	 * via call with getSetting() and have the input to _getSetting escaped for him.
	 *
	 * You can of course bypass __call() by calling the function _getSetting() directly
	 * but only when it is defined as public.
	 */
	public function __call($method, $arg)
	{
		foreach ($arg as $id => $value) {
			if (ctype_digit($value)) {
				$arg[$id] = (int)$value;
			} else if (is_string($value)) {
				$arg[$id] = $this->_db->escape_string($value);
			}
		}
		$method = '_'.$method;
		if (method_exists($this, $method)) {
		    return call_user_func_array(array($this, $method), $arg);
		} else {
		    throw new Exception('Tried to call non-existant method ' . __CLASS__ . '::' . $method.'()');
		}
	}
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

Me wrote:This makes a very good point actually. I can reduce this to a single parameter :P
I'm going to go and do that now :)
Ahh. No that won't work because of that __call() bit.
For those of you that didn't follow the call to _createSiteWithVideo() is preprocessed by __call() to escape all the parameters first so inside _createSiteWithVideo() I don't have to worry about escaping at all because it has all already been done.

Currently the call to _createSiteWithVideo is done like this:

Code: Select all

$newSite = $this->db->createSiteWithVideo($f->fsIdentity->txtTitle->getInput(),
                                          $f->fsIdentity->selClient->getInput(),
                                          $f->fsIdentity->selSubclient->getInput(),
                                          $f->fsQuestionnaire->getInput(),
                                          $f->fsVideo->txtMsgFeedback->getInput(),
                                          $f->fsVideo->txtMsgStream->getInput(),
                                          $f->fsVideo->txtMsgDownload->getInput(),
                                          $f->fsQuestionnaire->txtMsgAtStart->getInput(),
                                          $f->fsQuestionnaire->txtMsgOnceFinished->getInput(),
                                          $f->fsVideo->txtUrlDownload->getInput(),
                                          $f->fsVideo->txtUrlStream->getInput(),
                                          $f->fsVideo->txtWidth->getInput(),
                                          $f->fsVideo->txtHeight->getInput());
So obviously the first thing I thought when I read this...
Don't pass in everything the method needs; pass in enough so that the method can get to everything it needs.
...which is indeed very good advice, was to do this instead...

Code: Select all

$newSite = $this->db->createSiteWithVideo($f);
Obviously that's much nicer (even if you are just moving all those _getInput() call to inside _createSiteWithVideo()) you still don't need a massive argument list. Only problem is __call() doesn't have any strings to escape now :(. So I have to revert back to my original way. Doesn't matter really like Hockey said it isn't that bad.
User avatar
Nathaniel
Forum Contributor
Posts: 396
Joined: Wed Aug 31, 2005 5:58 pm
Location: Arkansas, USA

Post by Nathaniel »

Perhaps instead of $f, pass it $f->fsIdentity, $f->fsVideo, and $f->fsQuestionnaire.
User avatar
Ollie Saunders
DevNet Master
Posts: 3179
Joined: Tue May 24, 2005 6:01 pm
Location: UK

Post by Ollie Saunders »

nah dw i've come up with another solution, much better :)
Post Reply