current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 4 down vote favorite

I'm working on an old PHP website, and NetBeans is complaining about an uninitialized variable. I'm seeing some code like this:

$app->soapAPIVersion($apiVersion);
$_SESSION['apiVersion'] = $apiVersion;

The function looks something like this:

function soapAPIVersion(&$apiVersion)
{
    $apiVersion = '';
    $result = false;
    $soapResult = $client->call('getAPIVersion', array('sessionKey' => $sessionKey), $url);

    if (is_string($soapResult))
    {
        $apiVersion = $soapResult;
        $result = true;
    }

    return $result;    
}

I believe it's using this line to initialize the $apiVersion variable:

$app->soapAPIVersion($apiVersion);

For better coding practice, should that really be this:

$apiVersion = '';
$app->soapAPIVersion($apiVersion);

Or is it a valid trick?

share|improve this question
    
While I agree with the advice to refactor and avoid the reference, sometimes you have no choice (e.g. the built-in function preg_match). Yes, this is a bug in NetBeans, and there's no reason to initialize $apiVersion before calling the function. You will only get a warning if you read its value before initializing it. –  David Harkness Apr 14 at 6:54
add comment

2 Answers

up vote 4 down vote accepted

With all do respect, this code really did hurt. Netbeans is complaining, because the function soapAPIVersion expects a reference to a variable. You're passing an undeclared, and uninitialized variable to the function. Sort of like a void pointer/null-pointer thing.
It's not really a big deal, but I cannot, for the life of me, get why this function uses a reference as an argument. I think it better to change it to:

function soapAPIVersion()
{
    $soapResult = $client->call('getAPIVersion', array('sessionKey' => $sessionKey), $url);
    if (is_string($soapResult))
    {
        return $soapResult;
    }
    //implicitly return null, or throw exception
}

And call it like so:

$apiVersion = $app->soapAPIVersion();
if ($apiVersion === null)
{
    throw new RuntimeException('unable to get the API version');
}
$_SESSION['apiVersion'] = $apiVersion;

Bottom line: only pass by reference if you need your function to do 2 things at the same time, and you can't split those two things over 2 functions. situations are Very rare. Since PHP5, I've only had to use a reference argument 5~10 times, I think, so avoid, if at all possible

share|improve this answer
1  
Excellent, fully agree, thankyou! Unfortunately what you saw was just the very tip of the iceberg. This code base has more of the same and even worse examples. –  user unknown Aug 8 '13 at 21:17
    
@user2640336: In that case, even though I'm not religious, I'll pray for your sanity :) –  Elias Van Ootegem Aug 9 '13 at 7:03
add comment
up vote 5 down vote

  1. Clean Code, Chapter 3. Functions has some guidance:

    Have No Side Effects

    Side effects are lies. Your function promises to do one thing, but it also does other hidden things.

    [...]

    Anything that forces you to check the function signature is equivalent to a double-take. It’s a cognitive break and should be avoided.

    The side effect here is that the function changes the argument. A more readable approach would be returning the API version (instead of true/false) or throwing an exception in case of errors.

  2. $app->soapAPIVersion($apiVersion);
$_SESSION['apiVersion'] = $apiVersion;

    Note that the code should check the return value of soapAPIVersion and handle false somehow.

  3. The soapAPIVersion name is unclear. What does it do with the API version? From Clean Code, Chapter 2: Meaningful Names:

    Method Names

    Methods should have verb or verb phrase names like postPayment, deletePage, or save. Accessors, mutators, and predicates should be named for their value and prefixed with get, set, and is according to the javabean standard.

    I'd rename it to getApiVersion or fetchApiVersion.

    Note the camelCase too which is usually easier to read. From Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions:

    While uppercase may be more common, a strong argument can made in favor of capitalizing only the first letter: even if multiple acronyms occur back-to-back, you can still tell where one word starts and the next word ends. Which class name would you rather see, HTTPURL or HttpUrl?

share|improve this answer
1  
Great answer, thankyou! –  user unknown Aug 8 '13 at 21:15
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.