开发者

refactoring this block

开发者 https://www.devze.com 2023-02-01 13:23 出处:网络
I\'m refactoring some code that wasn\'t written by me. This block sets the value of $val but I want to clean it up a bit. Obviously I can\'t use the tertiary operator here. What other ways I can make

I'm refactoring some code that wasn't written by me. This block sets the value of $val but I want to clean it up a bit. Obviously I can't use the tertiary operator here. What other ways I can make this code cleaner?

  if (isset($var开发者_StackOverflows[$input])) {
       $val = $vars[$input];
  } elseif (isset($this->getI['io'])) {
       $val = $this->getI['io'];
  } elseif (isset($vars[5])) {
       $val = $vars[5];
  } else {
       $val = 10;
  }


$val = 10;
if (isset($vars[$input])) {
    $val = $vars[$input];
} elseif (isset($this->getI['io'])) {
    $val = $this->getI['io'];
} elseif (isset($vars[5])) {
    $val = $vars[5];
}

This is about as simple as it gets without obfuscating the code. I'd rather try to simplify the logic, it's kinda hard to comprehend why the value is being looked for in so many different places.


I'm afraid I don't know php. I'm assuming that if you were to pass (say) $vars[$input] to a function, by the time it was a parameter to the function, the parameter's set-ness would be true (if that's not the case, I'd try writing a function that tested isset() on its parameter and set $val if so). I find elseif's to add complexity; I try to avoid them. In this case, I would write a function that returned the value; then all my elseif's can become plain if's.

f() {
    if (isset($vars[$input])) {
        return $vars[$input];
    }
    if (isset($this->getI['io'])) {
        return $this->getI['io'];
    }
    if (isset($vars[5])) {
        return $vars[5];
    }
    return 10;
}

And, of course, in your calling function, assign $val to the result of this function.


In my opinion, your example is as clean as it gets. Sure, you could write it as a huge one-liner using the ternary operator:

$val = isset($vars[$input]) ? $vars[$input] : isset($this->getI['io'] ? $this->getI['io'] : isset($vars[5]) ? $vars[5] : 10;

But this is obviously much harder to read and to maintain, so the original example is definitely cleaner (although it might be missing some comments).


I don't know...it seems to be pretty concise, as is.

If you know what it does, it does it well and it is clean enough that you can figure it out again in the future, I say don't touch it.


While you're at it figure out what it's doing and add some comments.

e.g. why assign it to the magic number 10? maybe the context of the rest of it may shed some light.

As far as code goes, you're not going to get it any simpler than this.

0

精彩评论

暂无评论...
验证码 换一张
取 消