Sometimes depending on which user type if viewing my page, I need to add in a JOIN, or even just limit the results. Is there a cleaner way of going about it? Should I have separate statements for each type of request instead? What is more "proper"?
Here is what my code ends up looking like:
// Prepare statement
$stmt = $this->db->prepare('
SELECT *
FROM Documents
LEFT JOIN Notes ON ID = D_ID
'.($user_id ? "INNER JOIN Users ON UID = ID AND UID = :userid" : '')."
". ($limit ? 'LIMIT :offset, :limit' : '')
);
// Bind optional paramaters
if ($user_id) $stmt->bindParam(':userid', $user_id, DB::PARAM_INT);
if ($limit)
{
$stmt->bindParam(':offset', $limit[0], DB::PARAM_INT);
$stmt->bindParam(':limit', $limit[1], DB::PARAM_INT);
}
I'd create separate (protected) functions, those return a prepared statement that only needs to be executed.
/**
* @returns PDOStatement
*/
protected function prepareStatementForCase1(PDO $dbObject,Object $dataToBind){...}
/**
* @returns PDOStatement
*/
protected function prepareStatementForCase2(PDO $dbObject,Object $dataToBind){...}
Then, I would decide outside, which one has to be called. You can rebuild, maintain and read the code more easily.
Example:
class Document{
protected $dbObject;
public function __construct(PDO $dbObject){
$this->dbObject=$dbObject;
}
public function doQuery($paramOne,$paramTwo,...){
$logicalFormulaOne=...; // logical expression here with parameters
$logicalFormulaTwo=...; // logical expression here with parameters
if($logicalForumlaOne){
$dbStatement=$this->prepareStatementForCase1($dataToBind);
}else if($logicalFormuleTwo){
$dbStatement=$this->prepareStatementForCase2($dataToBind);
}
$dbResult=$dbStatement->execute();
}
protected function prepareStatementForCase1(Object $dataToBind){
$dbStatement=$this->dbObject->prepare("query string");
$dbStatement->bindParam(...);
return $dbStatement;
}
}
But I would not advice this, when your PDOResult object represents different type of database tuples, or when you return more rows in one of the cases.
What I usually do is that I create a class which represents (in your example) a Document. Only one. I can insert, delete, select, modify by its fields, and handle one item. When I need to (for example) fetch more of them, I create a new class, e.g. DocumentList, which handles a collection of documents. This class would give me an array of Document objects when it fetches more of them.
Maybe just wrap the insert strings into their own methods for clarity, like getUserInsertString($user_id)
, and try to make your quote use more consistent.
Also, are you testing whether $user_id
and $limit
are defined just by going if ($user_id)
? If so, if you had error reporting turned to all, you would get a bunch of undefined variable warnings. You may want to consider using if (isset($user_id))
instead.