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

まず、いくつかの標準的なリファクタリングを行います。なぜ私がそのようにするのか分かりませんが、ここに何がありますか？

  1. プロパティを次のようなローカル変数に置き換えます

    $machine = $this->machine;

  2. 条件についても同じことが機能しますが、条件を詳しく見ると、変数ごとに2つの状態しかないことが明らかになります。したがって、これは実際には変数ごとに1つの条件のみであり（タイプジャグリングを参照）、結果はtrueまたはになり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ブロックが行列に変換されました。関数は、その1つの状態にマップされています。

フィールドの名前を失いますが、コメントでそれを解決できます。

    $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();

これが含まれるクラスが値オブジェクトを表す場合は、決定を独自のタイプに移動してから、その決定タイプを単一のメソッドオブジェクトとして使用することをお勧めします。後で、さまざまな決定セットをより簡単に実行できるようになります。

于 2012-10-30T14:31:59.710 に答える
2

たぶん、あなたは本当にこれらすべての決定を必要とします、しかしあなたはそれを少し読みやすくすることができます。$this->machine == ''ifステートメント内に何度も書き込む代わりに、最初にこの値を意味のある変数に設定できます。

$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();
}
...

''この例では、2つのことを想定しています。最初に、値と未設定の両方を処理したいと考えてい0ます。値を使って何かをしたことはないので、これについてはよくわかりません。0

次に、ある関数が呼び出された場合、それ以上関数を呼び出したくないと想定しているので、次のifの前にelseを追加しました。

私の意見では、ifステートメントをネストすると、コードが読みにくくなります。これは、現在のifステートメントのレベルを覚えておく必要があるためです。

別のアプローチは、決定マトリックスを使用することです。可能なすべての組み合わせを保持する配列を作成でき、各組み合わせは関数名を知っています。

$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 に答える
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()、多くの人が同じ条件を共有しているため、グループ化から始めることができます。たとえば、たくさんの代わりに

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

それらをすべてグループ化できます。

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

次に、次の「レベル」の条件に進みます。これによってsの総数が減ることはないかもしれませんがif()、コードは現在よりもはるかに読みやすくなります。

于 2012-10-30T12:30:01.217 に答える
0

ご覧のとおり、メソッドに名前を付ける規則があります。そのようなコードとそのような規則がある場合は、名前でメソッドを呼び出すことを使用してリファクタリングします。これにより、コードは非常に短く、読みやすく、保守しやすくなります。

   $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 に答える