壞PHP類功能設計

Question

一邊看書我碰到下面的函數來了：

/* 
Update records in the database 
@param String $table the table being updated 
@param Array $changes array of changes field => value 
@param String $condition the condition 
@return Boolean 
*/ 
public function updateRecords($table, array $changes, $condition) 
{ 
    $update = "UPDATE " . $table . " SET "; 
    foreach($changes as $field => $value) 
    { 
     $update .= "`" . $field . "` = '{$value}', "; 
    } 
    //remove trailing , (comma) 
    $update .= substr($update, 0, -1); 

    if($condition != '') 
    { 
     $update .= "WHERE " . $condition; 
    } 
    $this->executeQuery($update); 
    //Not sure why it returns true. 
    return true; 
} 

糾正我，如果我錯了，但是這是不是一個糟糕的設計功能，完全沒有數據過濾/檢查。而且大部分函數總是返回「真」。

Answer 1

正確。它似乎是通過字符串連接構建一條SQL語句，而沒有任何衛生條件。這可能是可以接受的，如果所有的輸入都可以信任，但鑑於它是一個public函數我不認爲是這樣。

的return true似乎是多餘的，因爲它永遠不會返回別的 - 作者可能已經預期它作爲一種方法，以確保完成的功能，這意味着它使用失敗，悄無聲息，而不是調用die()或拋出異常的代碼。

風格方面，作者與串聯不一致：在某些地方，它與.運算符是直接並置的，在其他地方使用的是$placeholders。

如果$changes爲空，那麼對substr的調用將失敗，在構建列表後進行修剪是用逗號分隔列表項的錯誤方法。這是我會做（除了從未與SQL，當然）：

$first = true; 
foreach($changes as $field => $value) { 
    if(!$first) $update .= ", "; 
    $first = false; 
    $update .= "`$field` = '{$value}'"; // NEVER DO THIS 
} 

關於它的思考 - 良好此功能是它的良好記錄的唯一的事情。