このPHPコードを簡略化または改善できますか？

Question

次のコードをリファクタリング/改善/簡略化できますか？別のプログラマーの視点/視点が欲しいので、私はここにこれを掲示しています。他人が何をするのかを見ることは常に良いことです。

<?php 

function moderateTopic($topic_id, $action = NULL) { 
    $locked_query  = "SELECT topic_id FROM forum_topics WHERE status = 'locked' AND topic_id = '{$topic_id}'"; 
    $locked_count  = totalResults($locked_query); 
    $announcement_query = "SELECT topic_id FROM forum_topics WHERE topic_type = 2 AND topic_id = '{$topic_id}'"; 
    $announcement_count = totalResults($announcement_query); 
    $sticky_query  = "SELECT topic_id FROM forum_topics WHERE topic_type = 3 AND topic_id = '{$topic_id}'"; 
    $sticky_count  = totalResults($sticky_query); 

    if (is_null($action) == FALSE) { 
     switch ($action) { 
      case 1: 
       if ($locked_count > 0) { 
        $topic_query = "UPDATE forum_topics SET status = 'unlocked' WHERE topic_id = '{$topic_id}'"; 
       } else { 
        $topic_query = "UPDATE forum_topics SET status = 'locked' WHERE topic_id = '{$topic_id}'"; 
       } 
       doQuery($topic_query); 
       break; 
      case 2: 
       if ($announcement_count > 0) { 
        $topic_query = "UPDATE forum_topics SET topic_type = 1 WHERE topic_id = '{$topic_id}'"; 
       } else { 
        $topic_query = "UPDATE forum_topics SET topic_type = 2 WHERE topic_id = '{$topic_id}'"; 
       } 
       doQuery($topic_query); 
       break; 
      case 3: 
       if ($sticky_count > 0) { 
        $topic_query = "UPDATE forum_topics SET topic_type = 1 WHERE topic_id = '{$topic_id}'"; 
       } else { 
        $topic_query = "UPDATE forum_topics SET topic_type = 3 WHERE topic_id = '{$topic_id}'"; 
       } 
       doQuery($topic_query); 
       break; 
      case 4: 
       header('Location: ' . urlify(9, $topic_id)); 
       break; 
      case 5: 
       header('Location: ' . urlify(11, $topic_id)); 
       break; 
     } 
     header('Location: ' . urlify(2, $topic_id)); 
    } else { 
     $locked  = $locked_count > 0 ? 'Unlock' : 'Lock'; 
     $announcement = $announcement_count > 0 ? 'Unannounce' : 'Announce'; 
     $sticky  = $sticky_count > 0 ? 'Unsticky' : 'Sticky'; 

     return <<<EOT 
<div style="float: right;"> 
<form method="POST"> 
<select name="action" onChange="document.forms[0].submit();"> 
<option value="">- - Moderate - -</option> 
<option value="1"> =&gt; {$locked}</option> 
<option value="2"> =&gt; {$announcement}</option> 
<option value="3"> =&gt; {$sticky}</option> 
<option value="4"> =&gt; Move</option> 
<option value="5"> =&gt; Delete</option> 
</select> 
</form> 
</div> 
EOT; 
    } 
} 

?> 

Answer 1

私は重複を探し始めるでしょう。クエリ文字列の6つの非常に類似した出現を見る：

$topic_query = "UPDATE forum_topics SET status = 'unlocked' WHERE topic_id = '{$topic_id}'"; 
$topic_query = "UPDATE forum_topics SET status = 'locked' WHERE topic_id = '{$topic_id}'"; 
$topic_query = "UPDATE forum_topics SET topic_type = 1 WHERE topic_id = '{$topic_id}'"; 
$topic_query = "UPDATE forum_topics SET topic_type = 2 WHERE topic_id = '{$topic_id}'"; 
$topic_query = "UPDATE forum_topics SET topic_type = 1 WHERE topic_id = '{$topic_id}'"; 
$topic_query = "UPDATE forum_topics SET topic_type = 3 WHERE topic_id = '{$topic_id}'"; 

これらのそれぞれの後にクエリが実行されます。それでは方法はどうですか？

function setField($topic_id, $field, $value) { 
    $topic_query = "UPDATE forum_topics SET '{$field}' = '{$value}' WHERE topic_id = '{$topic_id}'"; 
    doQuery($topic_query); 
} 

は、今すぐあなたのswitch文の最初のビットは次のようになります。

switch ($action) { 
    case 1: 
     if ($locked_count > 0) { 
      setField($topic_id, 'status', 'unlocked'); 
     } else { 
      setField($topic_id, 'status', 'locked'); 
     } 
     break; 
    case 2: 
     if ($announcement_count > 0) { 
      setField($topic_id, 'topic_type', 1); 
     } else { 
      setField($topic_id, 'topic_type', 2); 
     } 
     break; 
    case 3: 
     if ($sticky_count > 0) { 
      setField($topic_id, 'topic_type', 1); 
     } else { 
      setField($topic_id, 'topic_type', 3); 
     } 
     break; 

しかし、私はねじ込みまし見ることまで - と - 時々フィールドは、文字列、そして時にはintですintのケースはすべてtopic_typeです。したがって、次のように動作させるようにしてください。

switch ($action) { 
    case 1: 
     if ($locked_count > 0) { 
      setField($topic_id, 'status', 'unlocked'); 
     } else { 
      setField($topic_id, 'status', 'locked'); 
     } 
     break; 
    case 2: 
     if ($announcement_count > 0) { 
      setTopicType($topic_id, 1); 
     } else { 
      setTopicType($topic_id, 2); 
     } 
     break; 
    case 3: 
     if ($sticky_count > 0) { 
      setTopicType($topic_id, 1); 
     } else { 
      setTopicType($topic_id, 3); 
     } 
     break; 

あなたはすぐにコードの状態に満足していることがわかります。