views:

78

answers:

6

Hello there!

Given this ugly method:

public function convert_cell_value( $val, $type )
{
    if($type == 'String')
    {
        return $val;
    }
    elseif($type == 'Number')
    {
        if($val - intval($val) > 0)
        {
            return $val;
        }
        else
        {
            return intval($val);
        }
    }
    else
    {
        return $val;
    }
}

Now my ten billion $ question is: when i should return values (not in this method but any other like this) to apply the DRY principles and go for performance too. Or: There's something wrong with my thought about performance and it's nothing to do with it when i return some value immediately?

Bonus question: Is there a simpler trick to get decimals than this?

if($val - intval($val) > 0)
{
    return $val;
}

Thanks for your precious time, fabrik

+6  A: 

You can simplify your method logic to this:

public function convert_cell_value( $val, $type )
{
    if ($type === 'Number' && ($ret = intval($val)) == $val) {
        return $ret;
    }
    return $val;
}

Or if you want to add more types, use a switch:

public function convert_cell_value( $val, $type )
{
    switch ($type) {
    case 'Number':
        if (($ret = intval($val)) == $val) {
            return $ret;
        }
    case 'String':
    default:
        return $val;
    }
}

You could also use just one return and replace return $ret by $val = $ret to return the right value.

Gumbo
Thank you for your answer, Gumbo. Clean and readable yet doing the trick! :)
fabrik
@fabrik: Thanks, you’re welcome!
Gumbo
+1  A: 

As your code will go only through one option, I would store the return value in a variable and return it at the end of the method.

PJP
A: 

As a rule, I prefer to have only 1 return per function for readability. Having a variable that is returned is 99,99% as fast as returning directly and is much more simple to debug.

// code from Gumbo
public function convert_cell_value( $val, $type )
{
    if ($type === 'Number' && ($ret = intval($val)) == $val) {
        $val = $ret; // this is the extra assigment
    }
    return $val; // only 1 return
}
Elzo Valugi
A: 

It depends on the function. If you have a function which does some different (and heavy performance cost) checks and the return value should be false if one check fails, it makes sense to return it immediately.

If it is just some "if (condition1) else " or a simple switch i would just return it in the end due to better readability.

TheCandyMan666
+2  A: 

Your function here could be refactored as:

function convert_cell_value($val, $type)
{
  if ($type == 'Number') 
    return intval($val);
  else
    return $val;
}

In practice, returning values is seldom subject to DRY since "return" is a minor redundancy that can usually only be replaced by assigning to a variable several times and returning that variable once.

What could be an argument against having multiple return statements is SESE (single entry single exit) which states there should only be one return statement, for readability reasons (you might miss one) and cleanup reasons (you have to clean up any allocated resources before you return).

In a situation like yours, your function structure is effectively "decide what to return", so the readability argument does not apply.

Victor Nicollet
+1 because of your clear description about DRY and SESE. This is something i've never heard of :o Thank you Victor
fabrik
:) you're welcome
Victor Nicollet
+1  A: 

Use ternary and implicit cast.

public function convert_cell_value($val, $type) {
    return $type === 'Number' && !is_float($val + 1) ? intval($val): $val;
}

Also, if your checking if the value has decimal, use is_float instead. intval produces a strict integer value. So if you have the value below and compare, the result would be false even though it should be true.

intval('420000000000000000000'); // 2147483647
sheeks06
+1 because of extreme compact solution. So oneliner like method itself is meaningless :) Thank you!
fabrik