5
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ Apr 14, 2014 at 6:54

2 Answers 2

4
\$\begingroup\$

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

\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ Aug 8, 2013 at 21:17
  • \$\begingroup\$ @user2640336: In that case, even though I'm not religious, I'll pray for your sanity :) \$\endgroup\$ Aug 9, 2013 at 7:03
5
\$\begingroup\$
  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?

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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