3

我有很多 IF 句子，每个句子都启动一个函数。
有没有一种明显的方法可以更简单地编写这段代码？
每个 IF 启动不同的功能，但它仍然看起来有点矫枉过正。

    if ($this->machine == '' AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like == '' AND $this->article_or_tool == '') {
        $this->AllTime();
    }
    if ($this->machine <> 0 AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like == '' AND $this->article_or_tool == '') {
        $this->ByMachine();
    }
    if ($this->machine == '' AND $this->date_from <> 0 AND $this->date_to <> 0 AND $this->date_like == '' AND $this->article_or_tool == '') {
        $this->ByDate();
    }
    if ($this->machine <> 0 AND $this->date_from <> 0 AND $this->date_to <> 0 AND $this->date_like == '' AND $this->article_or_tool == '') {
        $this->ByMachineByDate();
    }
    if ($this->machine == '' AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like <> 0 AND $this->article_or_tool == '') {
        $this->ByDateLike();
    }
    if ($this->machine <> 0 AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like <> 0 AND $this->article_or_tool == '') {
        $this->ByMachineByDateLike();
    }
    if ($this->machine == '' AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like == '' AND $this->article_or_tool <> 0) {
        $this->ByArticle();
    }
    if ($this->machine <> 0 AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like == '' AND $this->article_or_tool <> 0) {
        $this->ByMachineByArticle();
    }
    if ($this->machine == '' AND $this->date_from <> 0 AND $this->date_to <> 0 AND $this->date_like == '' AND $this->article_or_tool <> 0) {
        $this->ByDateByArticle();
    }
    if ($this->machine == '' AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like <> 0 AND $this->article_or_tool <> 0) {
        $this->ByDateLikeByArticle();
    }
    if ($this->machine <> 0 AND $this->date_from <> 0 AND $this->date_to <> 0 AND $this->date_like == '' AND $this->article_or_tool <> 0) {
        $this->ByMachineByDateByArticle();
    }
    if ($this->machine <> 0 AND $this->date_from == '' AND $this->date_to == '' AND $this->date_like <> 0 AND $this->article_or_tool <> 0) {
        $this->ByMachineByDateLikeByArticle();
    }

解决方案
这是我重构后的代码：

function MethodPicker() {
    $machine            = $this->machine            <> 0;
    $date_from          = $this->date_from          <> 0;
    $date_to            = $this->date_to            <> 0;
    $date_like          = $this->date_like          <> 0;
    $article_or_tool    = $this->article_or_tool    <> 0;

    $decision  = array($machine, $date_from, $date_to, $date_like, $article_or_tool);
    $decisions = array(
                    'AllTime' =>                        array(false,    false,  false,  false,  false   ),
                    'ByMachine' =>                      array(true,     false,  false,  false,  false   ),
                    'ByDate' =>                         array(false,    true,   true,   false,  false   ),
                    'ByMachineByDate' =>                array(true,     true,   true,   false,  false   ),
                    'ByDateLike' =>                     array(false,    false,  false,  true,   false   ),
                    'ByMachineByDateLike' =>            array(true,     false,  false,  true,   false   ),
                    'ByArticle' =>                      array(false,    false,  false,  false,  true    ),
                    'ByMachineByArticle' =>             array(true,     false,  false,  false,  true    ),
                    'ByDateByArticle' =>                array(false,    true,   true,   false,  true    ),
                    'ByDateLikeByArticle' =>            array(false,    false,  false,  true,   true    ),
                    'ByMachineByDateByArticle' =>       array(true,     true,   true,   false,  true    ),
                    'ByMachineByDateLikeByArticle' =>   array(true,     false,  false,  true,   true    ),
    );
    $method = array_keys($decisions, $decision, true);
    $method && list($method) = $method;
    $method && $this->$method();
}
4

5 回答 5

2

Maybe you really need all this decisions, but you could make it a bit easier to read. Instead of writing many times $this->machine == '' inside the if statement, you could set this value to a meaningful variable first.

$machineIsEmpty = $this->machine == '' || $this->machine == 0;
$dateFromIsEmpty = $this->date_from == '' || $this->machine == 0;
...

if ($machineIsEmpty && $dateFromIsEmpty && $dateToIsEmpty && $dateLikeIsEmpty && $articleOrToolIsEmpty)
{
  $this->AllTime();
}
else if (!$machineIsEmpty && $dateFromIsEmpty && $dateToIsEmpty && $dateLikeIsEmpty && $articleOrToolIsEmpty)
{
  $this->ByMachine();
}
...

In this example i assume two things: first i suspect that you want to handle both the values '' and 0 as not set. I'm not sure about this, because there is no case where you did anything with the value 0.

Second i assume that if one function was called, you do not want to call further functions, so i added an else before the next if.

Nesting the if statements would in my opinion make the code more difficult to read, because you have to remember in which level of if statement you currently are.

An alternative approach would be to use a decision-matrix. You can write an array holding all possible combinations, and each combination knows the function name.

$myObject = new TestClass();
$myObject->DoAction($machineIsSet, $dateFromIsSet, $dateToIsSet, $dateLikeIsSet, $articleToolIsSet);

class TestClass
{
  private $actionMatrix = array(
    //    machine, dateFrom, dateTo, dateLike, articleOrTool, action
    array(false,   false,    false,  false,    false,         'AllTime'),
    array(true,    false,    false,  false,    false,         'ByMachine')
  );

  public function DoAction($machine, $dateFrom, $dateTo, $dateLike, $articleOrTool)
  {
    foreach($this->actionMatrix as $action)
    {
      if (($action[0] == $machine) && ($action[1] == $dateFrom) && ($action[2] == $dateTo) && ($action[3] == $dateLike) && ($action[4] == $articleOrToolLike))
      {
        $functionName = $action[5];
        $this->$functionName(); // call the function
        break;
      }
    }
    // no action found, maybe we want some error handling here?
  }

  public function AllTime()
  {
    echo('AllTime');
  }

  public function ByMachine()
  {
    echo('ByMachine');
  }
}
于 2012-10-30T12:56:33.043 回答
2

首先，我会做一些标准的重构。不知道为什么我这样做，但这里是：

  1. 用局部变量替换属性，例如

    $machine = $this->machine;

  2. 同样适用于条件，但是看起来更接近条件很明显，每个变量只有两个状态，所以这实际上是每个变量只有一个条件（参见Type Juggling），它导致trueor false。分配条件，而不是：

    $machine = $this->machine == '' || $this->machine == 0;

（学分去martinstoeckli正确的条件）

这将是一个开始。到现在为止的 if 子句已经改变并且会更紧凑。但是，为什么要停在这里？有一个当前的决定：

$decision  = [$machine, $date_from, $date_to, $date_like, $article_or_tool];

并且有一组决策可供选择：

$decisions = [
    'AllTime' => [true, true, true, true, true],
    ...
];

所以需要做的就是找到决策并执行方法：

$method = array_keys($decisions, $decision, true);
$method && $this->$method();

if 块已变成矩阵。该函数已映射到它的一种状态。

您丢失了字段上的名称，但是，您可以通过评论解决这个问题：

    $decisions = [
        //            machine  from  to    like  article
        'AllTime' => [true   , true, true, true, true],
        ...
    ];

乍看上去：

$machine = $this->machine == '' || $this->machine == 0;
... # 4 more times

$decision  = [$machine, $date_from, $date_to, $date_like, $article_or_tool];

$decisions = [
    'AllTime' => [true, true, true, true, true],
    ... # 11 more times
];

$method = array_keys($decisions, $decision, true);
$method && $this->$method();

如果 this 所在的类表示一个值对象，我建议您将决策移入它自己的类型，然后将该决策类型用作单个方法对象。将使您以后能够更轻松地做出不同的决策。

于 2012-10-30T14:31:59.710 回答
1

其中很多是重复的，因此您可以嵌套条件以消除冗余，例如：

if ($this->machine == '') {
  // do everything requiring empty 'machine' string
  // remove condition from all subsequent ifs in this context
} else if ($this-> machine <> 0) {

}

我没有时间也没有兴趣浏览所有这些代码并实际为您执行此操作，但这个想法应该为您提供足够的信息，以便您将其作为练习来实现给读者。（：

于 2012-10-30T12:29:21.097 回答
0

有时，如果您的条件不同，这是不可能的，但在这种情况下，您可以从分组您if()的 s 开始，因为许多共享相同的条件。例如，而不是拥有大量

if( $this->machine <> 0 AND .... )

您可以将它们组合在一起：

if( $this->machine <> 0 ) {
    // all related ifs there
}

然后继续进行下一个“级别”的条件。虽然这可能不会减少 s 的总数if()，但您的代码将比现在更具可读性。

于 2012-10-30T12:30:01.217 回答
0

As I see you have a convention naming your methods. If I have such code and such conventions, I would refactor it using calling method by name, in this way your code will be very short, easy to read and maintainable

   $method = '';

    if (this->machine <> 0) $method.="ByMachine";
    if ($this->date_from <> 0 AND $this->date_to <> 0) $method.="ByDate"
    .....
    //here you have full $method name ByMachineByDateByArticle or ByMachineByDateByArticle etc
    if($method){ 
       call_user_func_array(array($this, $method));
    }
于 2012-10-30T12:57:02.887 回答