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:
- Give variables meaningful names from a domain rather than a programming perspective (so not
$key
and$value
, but$orderId
and$order
for instance) - Try to reduce the amount of indentation. The more indentation you have the more mental capacity is taken away from the code at hand
- Extract parts of this into separate private methods with meaningful names (see 1)
- Replace else-statements with early returns
- 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.