Page 1 of 1

The 13 parameter function

Posted: Sat Jul 29, 2006 6:40 am
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;
}

Posted: Sat Jul 29, 2006 11:00 am
by Ambush Commander
Erm... is this a plugin or something?

Posted: Sat Jul 29, 2006 11:12 am
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

Posted: Sat Jul 29, 2006 11:30 am
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? ;)

Posted: Sat Jul 29, 2006 11:47 am
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.

Posted: Sat Jul 29, 2006 12:36 pm
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

Posted: Sat Jul 29, 2006 12:37 pm
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...

Posted: Sat Jul 29, 2006 1:15 pm
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)

Posted: Sat Jul 29, 2006 3:29 pm
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.'()');
		}
	}

Posted: Sat Jul 29, 2006 5:49 pm
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.

Posted: Sat Jul 29, 2006 6:21 pm
by Nathaniel
Perhaps instead of $f, pass it $f->fsIdentity, $f->fsVideo, and $f->fsQuestionnaire.

Posted: Sat Jul 29, 2006 7:15 pm
by Ollie Saunders
nah dw i've come up with another solution, much better :)