1
\$\begingroup\$

I have a notification system. If someone posts a new comment to the topic you commented, then you get a notification. The problem is, that the code is looking awful. First I count how many notifications you have, so I can write it into the "notification bell", then I write the same code again, but this time to output the result (xy commented x hours ago...)

My main concern is that the same queries are running twice (or even more), and I'm asking if there is a way to run them only once?

So here's my code (well, my queries are bad too, because it's sorted by last comment date from me, instead of the last time someone commented, so the notifications area time order is also bad (it can happen, that a comment from 1 day ago is the first notification, when a comment from 1 hour ago is behind it)):

$sql = "SELECT p1.* FROM comment p1 INNER JOIN 
          (SELECT max(date) MaxPostDate, user_id 
           FROM comment WHERE user_id='$me' and deleted=0 
           GROUP BY topic_id, picture_id, news_id) p2 
        ON p1.user_id = p2.user_id AND p1.date = p2.MaxPostDate 
        WHERE p1.user_id='$me' and deleted=0 
        ORDER BY p1.date DESC "
$comment_query = sql_query($conn, $sql);
if(sql_num($comment_query)!=0)
{
while ($comment = sql_fetch($comment_query))
{
    if($comment['topic_id']!=0)
    {
        $current_forum = sql_fetch(sql_query($conn, "SELECT url, name
        FROM forum
        WHERE id='".$comment['topic_id']."' and deleted=0"));
        $current_comments = sql_fetch(sql_query($conn, "SELECT count(id) as count, date 
        FROM comment 
        WHERE deleted=0 and topic_id='".$comment['topic_id']."'"));
        $comment_topic_id = $comment['topic_id'];
        $comment_id = $comment['id'];
        $comment2_query = sql_fetch(sql_query($conn,"SELECT count(id) AS cid 
        FROM comment 
        WHERE topic_id=".$comment_topic_id ." and id<".$comment_id ." and deleted=0 "));
        $result = $comment2_query['cid'] + 1;
        if($comment['seen']=='0000-00-00 00:00:00') {
            $unread = $current_comments[0] - $result;
            if($unread!=0)
            {
                if((!empty($_GET['p'])) and $_GET['p']=='forum' and 
                $_GET['x']==$current_forum['url'])
                //If I'm at the specific url (I'm watching the new comments, so update it)
                {
                    $now = date('Y-m-d H:i:s');
                    sql_query($conn,"UPDATE comment SET seen='$now' WHERE user_id='$me' AND id='$comment_id' AND topic_id='.$comment_topic_id.' ");
                }
                else //increase number to add it to noficiation bell
                {
                    $count++;
                    $forum_notif++;
                }
        }
        else
        {
            $last_time_seen = $comment['seen'];
            $count_comments = sql_fetch(sql_query($conn,"SELECT count(id) AS cid
            FROM comment 
            WHERE topic_id=".$comment_topic_id." and deleted=0 and date>'.$last_time_seen.' "));
            if($count_comments['cid']!=0)
            {
                if((!empty($_GET['p'])) and $_GET['p']=='forum' and 
                $_GET['x']==$current_forum['url'])
                {

                    $now = date('Y-m-d H:i:s');
                    sql_query($conn,"UPDATE comment SET seen='$now' WHERE user_id='$me' AND id='$comment_id' AND topic_id='.$comment_topic_id.' ");
                }
                else
                {
                    $count++;
                    $forum_notif++;
                }
            }
        }
    }
    elseif($comment['picture_id']!=0)
    {
       //same thing again for a different type of forum...

Here's the one for outputting the "xy commented x hours ago..." part:

$sql = "SELECT p1.* FROM comment p1 INNER JOIN 
          (SELECT max(date) MaxPostDate, user_id 
           FROM comment WHERE user_id='$me' and deleted=0 
           GROUP BY topic_id, picture_id, news_id) p2 
        ON p1.user_id = p2.user_id AND p1.date = p2.MaxPostDate 
        WHERE p1.user_id='$me' and deleted=0 
        ORDER BY p1.date DESC "
$comment_query = sql_query($conn, $sql);
if(sql_num($comment_query)!=0)
{
while ($comment = sql_fetch($comment_query))
{
    if($comment['topic_id']!=0)
    {
        $current_forum = sql_fetch(sql_query($conn, "SELECT url, name
        FROM forum
        WHERE id='".$comment['topic_id']."' and deleted=0"));
        $current_comments = sql_fetch(sql_query($conn, "SELECT count(id) as count, date 
        FROM comment 
        WHERE deleted=0 and topic_id='".$comment['topic_id']."'"));
        $comment_topic_id = $comment['topic_id'];
        $comment_id = $comment['id'];
        $comment2_query = sql_fetch(sql_query($conn,"SELECT count(id) AS cid 
        FROM comment 
        WHERE topic_id=".$comment_topic_id ." and id<".$comment_id ." and deleted=0 "));
        $result = $comment2_query['cid'] + 1;
        $get_date = sql_fetch(sql_query($conn,"SELECT date
        FROM comment
        WHERE topic_id=".$comment_topic_id." ORDER BY id DESC"));
        if($comment['seen']=='0000-00-00 00:00:00') {
            $unread = $current_comments[0] - $result;
            if($unread!=0)
            {
                $new_number = $current_comments[0] - ($unread-1);
                if($current_comments[0]<=20) //20 comment appears on a page
                {
                    ?>
                    <p class="notif">
                    <a class="comments" href="/forum/
                    <?php print $current_forum['url']; ?>#<?php print $new_number; ?>">
                    <?php print $unread; ?> new comments at
                    <?php print ''.$current_forum['name'].' forum topic!
                    <span class="when_notif">'.since_time($get_date['date']).'</span>
                    </a>
                    </p>'; //x hours ago
                }
                else
                {
                    $limitation = 20;
                    $maxpage_comment = ceil($new_number / $limitation);//get page number
                    ?>
                    <p class="notif">
                    <a class="comments" href="/forum/<?php print $current_forum['url']; ?>
                    /<?php print $maxpage_comment; ?>#<?php print $new_number; ?>">
                     <?php print $unread; ?> new comments at
                     <?php print ''.$current_forum['name'].' forum topic!
                     <span class="when_notif">'.since_time($get_date['date']).'</span>
                     </a>
                     </p>';
                }
            }
        }
        else
        {
            $last_time_seen = $comment['seen'];
            $count_comments = sql_fetch(sql_query($conn,"SELECT count(id) AS cid
            FROM comment 
            WHERE topic_id=".$comment_topic_id." and deleted=0 and date>'.$last_time_seen.' "));
            if($count_comments['cid']!=0)
            {
                $new_number = $current_comments[0] - ($count_comments['cid']-1);
                if($current_comments[0]<=20)
                {
                    ?>
                    <p class="notif">
                    <a class="comments" href="/forum/
                    <?php print $current_forum['url']; ?>#<?php print $new_number; ?>">
                    <?php print $count_comments['cid']; ?> new comments at
                    <?php print ''.$current_forum['name'].' forum topic!
                     <span class="when_notif">'.since_time($get_date['date']).'</span>
                     </a>
                     </p>';
                }
                else
                {
                    $limitation = 20;
                    $maxpage_comment = ceil($new_number / $limitation);
                    ?>
                    <p class="notif">
                    <a class="comments" href="/forum/<?php print $current_forum['url']; ?>
                    /<?php print $maxpage_comment; ?>#<?php print $new_number; ?>">
                    <?php print $count_comments['cid']; ?> newcomments at
                    <?php print ''.$current_forum['name'].' forum topic!
                    <span class="when_notif">'.since_time($get_date['date']).'</span>
                    </a>
                    </p>';
                }
            }
        }
    }
    elseif($comment['picture_id']!=0)
    {
       //same thing again for a different type of forum...
\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Oct 23 '18 at 8:25
  • 2
    \$\begingroup\$ Start from formatting your code to make it fit to the screen (I've formatted the first query, you can follow the pattern for the rest) and giving your variables *meaningful names. We have no idea what does $temp mean, least what $temp2 does. \$\endgroup\$ – Your Common Sense Oct 23 '18 at 8:32
  • \$\begingroup\$ I've updated it. \$\endgroup\$ – Phoenixy Oct 23 '18 at 10:50
1
\$\begingroup\$

Your code is almost impossible to read, so instead of the code review I would answer the question you asked:

First I count how many notifications you have, so I can write it into the "notification bell", then I write the same code again, but this time to output the result (xy commented x hours ago...)

The answer is simple: well, run your code once. Instead of outputting your data directly to the screen, just prepare it for the output, having your calculations along. So just combine your two codes into one, but instead of direct output either collect the data into array to be output later (much more preferred) or concatenate the resulting HTML into a string variable.

Then you will be able to provide the number and then echo the actual list.

Regarding other issues, the most critical one is SQL injection. Your sql_query() function must have a support for prepared statements, to be called like this:

$sql = "SELECT url, name FROM forum WHERE id=? and deleted=0";
sql_query($conn, $sql, [$comment['topic_id']]);

it is not only much cleaner but also 100% safe (given your implementation of prepared statements is correct). The actual implementation depends on the underlying driver. In case you are using PDO, you can take it from my PDO wrapper, and for mysqli it would be something like

function sql_query($conn, $sql, $params = [])
{
    if ($params)
    {
        $stmt = $conn->prepare($sql);
        $types = str_repeat('s', count($params));
        $stmt->bind_param($types, ...$params);
        return $stmt->execute()->get_result();
    } else {
        $return = $conn->query($sql);
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks, with arrays my code is now 690 lines shorter. About SQL injection: There's no user input/update/insert into here, is it still voulnerable? Though I'll consider that, but it'll require a complete rewrite of the whole site. \$\endgroup\$ – Phoenixy Oct 25 '18 at 7:52
  • \$\begingroup\$ Yes. it is still vulnerable. SQL injection is not about whatever "user input", it's about SQL. The "user input" is too vague a term to be taken into account. You have to guard your SQL, not inputs. \$\endgroup\$ – Your Common Sense Oct 25 '18 at 9:17
1
\$\begingroup\$

I'd start by splitting out the logic into multiple functions so that the query result loop isn't also responsible for defining all the execution logic that happens within. It instead will call functions where that is defined.

E.g.

$current_forum = sql_fetch(sql_query($conn, "SELECT url, name
FROM forum
WHERE id='".$comment['topic_id']."' and deleted=0"));

Becomes

$current_forum = fetch_current_forum($conn, $comment['topic_id']);

And

function fetch_current_forum($connection, $topic) {
    return sql_fetch(sql_query($conn, "SELECT url, name
        FROM forum
        WHERE id='".$comment['topic_id']."' and deleted=0")
    );
}

That way common logic is encapsulated in a single area instead of being redefined in multiple areas. Function arguments can also help with small differences (like what's happening if($current_comments[0]<=20)).

P.S. Using variables directly in a SQL query is a security vulnerability (see SQL Injection). PDO can help there.

\$\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.