Gathering data from database

Question

The following code has one of the most confusing lines I've ever wrote. I can imagine ten other way to write it but I do not know which else could be any better.

Note:

Db() is very similar to PDO(), it just extends it adding few features I'm not using here.

Post::addExtra() add abstract datas elaborating database data.

For example he created a $data[13] = $data['from db1'] .' with '. $data['from db2']. These because they are going to be passed to the template.

$db = new Db();
$s = new Session();
# Default statement and parameters
$stmt = 
"SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 ORDER BY PostTime DESC
 LIMIT 0, 30";
$par = array();
# We change the statement if the tab is selected
if ($tab = get('tab')) {
 switch ($tab) {
 case 'admin':
 $stmt = 
 "SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 WHERE p.PostUID = 1
 ORDER BY PostTime DESC
 LIMIT 0, 30";
 break;
 case 'trusted':
 if ($s->isLogged()) {
 $stmt = 
 "SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 WHERE p.PostUID IN (
 SELECT TrustedUID
 FROM Trust
 WHERE TrusterUID = :uid
 )
 ORDER BY PostTime DESC
 LIMIT 0, 30";
 $par = array('uid' => $s->getUID());
 } else {
 $stmt = '';
 }
 break;
 case 'favorite':
 if ($s->isLogged()) {
 $stmt = 
 "SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 WHERE p.PostPID IN (
 SELECT FavoritePID
 FROM Favorites
 WHERE FavoriteUID = :uid
 )
 ORDER BY PostTime DESC
 LIMIT 0, 30";
 $par = array('uid' => $s->getUID());
 } else {
 $stmt = '';
 }
 break;
 case 'top':
 $weekAgo = time() - week;
 $monthAgo = time() - month;
 $stmt = 
 "SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 WHERE p.PostTime > $monthAgo
 LIMIT 0, 3
 UNION
 SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 WHERE p.PostTime > $weekAgo
 ORDER BY PostFlags DESC
 LIMIT 0, 30";
 break;
 case 'recent':
 default:
 break;
 }
} 
# Loading posts
try {
 $sql = $db->prepare($stmt);
 $sql->execute($par);
 $posts['Data'] = $sql->fetchAll();
} catch (PDOException $e) {
 throw new MyEx($e->getMessage());
}
if (count($posts['Data']) > 0) {
 foreach ($posts['Data'] as &$post) {
 $post = Post::addExtra($post);
 }
}

Answer 1

General SQL-related advice: I would factor most of these queries into a single view; you can then add WHERE parameters when selecting from the view. You can also remove the few instances of variable expansion inside the queries, and replace them with named parameters throughout (you have :uid already).

For example:

CREATE OR REPLACE VIEW PostsAnnotated AS
SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime,
 u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID
 ORDER BY PostTime DESC;
SELECT FROM PostsAnnotated WHERE PostUID = 1 LIMIT 30;
SELECT FROM PostsAnnotated WHERE PostUID IN (
 SELECT TrustedUID
 FROM Trust
 WHERE TrusterUID = :uid)
LIMIT 30;

As far as the code around addExtra: I wouldn't set $posts['Data'] to overwrite it immediately. Instead I would loop on the sql results and append to $posts['Data'] (IIRC the syntax is $posts['Data'][] = $next_elem).

• sh03: I didn't totally got the first part of your answer. Could you explain it a little bit more?
• Tobu: @Charlie does the SQL example help?
• sh03: I think i got it: The first part of the sql will be common to every query of the tab. Then I'll have to expand it with custom sql for every tab (such as SELECT FROM PostsAnnotated WHERE PostUID = 1 LIMIT 30;)?

Answer 2

It appears that the only difference between these queries lies in the where clause. If this is the case, you can clean up the code a bit by removing redundant SQL, like so:

$stmt = 
"SELECT p.PostPID, p.PostUID, p.PostText, p.PostTime, u.UserUID, u.UserName, u.UserImage, u.UserRep,
 (
 SELECT COUNT(*)
 FROM Flags as f 
 JOIN Posts as p1
 ON p1.PostPID = f.FlagPID
 WHERE p1.PostPID = p.PostPID
 ) as PostFlags
 FROM Posts AS p
 JOIN Users AS u
 ON p.PostUID = u.UserUID";
if ($tab = get('tab')) {
 switch ($tab) {
//...Snip...
 case 'trusted':
 if ($s->isLogged()) {
 $stmt = $stmt . 
 "WHERE p.PostUID IN (
 SELECT TrustedUID
 FROM Trust
 WHERE TrusterUID = :uid
 )";
 $par = array('uid' => $s->getUID());
 } else {
 $stmt = '';
 }
 break;
//...Snip...
 }
} 
$stmt . "ORDER BY PostFlags DESC LIMIT 0, 30";

It improves readability a bit, as only the parts that change from query to query are present.