Skip to content
Advertisement

How to refactor this function properly? [closed]

I have the following PHP function, that takes in two variables, checks their values using multiple if/else statements and manipulate their values. How can I make this piece of code more readable and easy to understand, i.e. refactor it?

    $myarr = somevalue();
    public function messy($var, $number) {
            if (is_array($var)) {
                foreach ($var as $i => $v) {
                    if (isset($myarr[$v])) {
                        if ($number == false || $number[$i] == "_NONE_" || $myarr[$v] == $number[$i]) {
                            unset($myarr[$v]);
                        }
                        else {
                            $myarr[$v] = $number[$i];
                        }
                    }
                    else {
                        if ($number != $check_value) {
                            $myarr = $myarr + array($v => $number[$i]);
                        }
                    }
                }
            }
            else {
                if (isset($myarr[$var])) {
                    if ($myarr[$var] == $number || $number == "_NONE_" || $number == false) {
                        unset($myarr[$var]);
                    }
                    else {
                        $myarr[$var] = $number;
                    }
                }
                else {
                    if ($number != false) {
                       $myarr = $myarr + array($var => $number);
                    }
                }
            }
        }

Advertisement

Answer

Some general tips for you:

  1. Give variables meaningful names from a domain rather than a programming perspective (so not $key and $value, but $orderId and $order for instance)
  2. Try to reduce the amount of indentation. The more indentation you have the more mental capacity is taken away from the code at hand
  3. Extract parts of this into separate private methods with meaningful names (see 1)
  4. Replace else-statements with early returns
  5. Wrap the data you’re modifying in a class and design it so you can modify it by calling methods on it

As a small example on extracting to private methods (3) you could do something like the following:

public function messy($var, $number) {
    if (is_array($var)) {
        $myarr = $this->handleArrayCase($var, $number);
    }
    else {
        $myArr = $this->handleNonArrayCase($var, $number);
    }
}

private function handleArrayCase($var, $number) {
    foreach ($var as $i => $v) {
        if (isset($myarr[$v])) {
            if ($number == false || $number[$i] == "_NONE_" || $myarr[$v] == $number[$i]) {
                unset($myarr[$v]);
            }
            else {
                $myarr[$v] = $number[$i];
            }
        }
        else {
            if ($number != $check_value) {
                $myarr = $myarr + array($v => $number[$i]);
            }
        }
    }

    return $myarr;
}

private function handleNonArrayCase($var, $number) {
    if (isset($myarr[$var])) {
        if ($myarr[$var] == $number || $number == "_NONE_" || $number == false) {
            unset($myarr[$var]);
        }
        else {
            $myarr[$var] = $number;
        }
    }
    else {
        if ($number != false) {
            $myarr = $myarr + array($var => $number);
        }
    }
    
    return $myarr;
}

Please note that this is not a working example and it does not follow the rule about meaningful names as I do not have enough context to do so.

User contributions licensed under: CC BY-SA
7 People found this is helpful
Advertisement