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
$keyand$value, but$orderIdand$orderfor 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.