5
\$\begingroup\$

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);
    }
}
\$\endgroup\$
4
\$\begingroup\$

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).

\$\endgroup\$
3
  • \$\begingroup\$ I didn't totally got the first part of your answer. Could you explain it a little bit more? \$\endgroup\$
    – Shoe
    Mar 26 '11 at 18:51
  • \$\begingroup\$ @Charlie does the SQL example help? \$\endgroup\$
    – Tobu
    Mar 27 '11 at 9:10
  • \$\begingroup\$ 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;)? \$\endgroup\$
    – Shoe
    Mar 27 '11 at 9:27
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.