3
\$\begingroup\$

I have a website that, based on a user's comments, filters comments by "most liked first, most disliked first" etc...

I have a custom table for comments scoring system and system working with +1 and -1's.

A few days ago I wanted to filter comments by most liked and most disliked as I said and wrote this code:

In my functions.php

function bestanswers($a){

    global $wpdb;

    $results = $wpdb->get_results( 'SELECT * FROM wp_comments LEFT JOIN wp_custom_scoring ON wp_custom_scoring.entryID = wp_comments.comment_ID WHERE class="point" and comment_post_ID='.intval($a).' AND type="plus" GROUP BY entryID ORDER BY COUNT(entryID) DESC');

    if(empty($results)){
        echo 'Its Empty!';
        return false;
    }else{
        return $results;
    }
}

And in my functions.php again:

function score($a,$b){

    if($b == "minus"){
    global $wpdb;
    $results = $wpdb->get_results( 'SELECT count(id) as total,type,class,entryID FROM wp_custom_scoring WHERE type="minus"  and class="point" and entryID='.intval($a));

        if($results[0]->total!= 0){
            return "-".$results[0]->total;
        }else{
            return 0;
        } 


    }elseif($b == "plus"){
    global $wpdb;
    $results = $wpdb->get_results( 'SELECT count(id) as total,type,class,entryID FROM wp_custom_scoring WHERE type="plus"  and class="point" and entryID ='.intval($a));

        if($results[0]->total != 0){
            return "+".$results[0]->total;
        }else{
            return 0;
        } 

    }else{
    global $wpdb;
    $results = $wpdb->get_results( 'SELECT count(id) as total,class,entryID FROM wp_custom_scoring WHERE  class="fav" and entryID='.intval($a));

    return $results[0]->total;
    }


}

And in comments.php:

if(isset($_GET['filter']) and sanitize_text_field(esc_attr($_GET['filter'])) == 'bestanswers'){

foreach (bestanswers($postID) as $bests) { 
if ( score($bests->comment_ID,"plus") + score($bests->comment_ID,"minus") > 0 ){
echo $bests->comment_ID;
comment_text($bests->comment_ID);
///...etc with my custom theme template
}

It's working and I'm able to get everything as I wanted but it's looking ugly to me. Is there are any better way to do this? Or any improvement on my code?

\$\endgroup\$
2
  • \$\begingroup\$ Welcome to code review. Your question may attract more attention if the title was just what the code does rather than what you want from the review. An example for this question might be Filtering web comments by votes. Please see the code review guidelines at codereview.stackexchange.com/help/how-to-ask. \$\endgroup\$ – pacmaninbw Jul 31 '19 at 12:32
  • 1
    \$\begingroup\$ Thanks for your response! You are right :) \$\endgroup\$ – techn9ne Jul 31 '19 at 13:06
1
\$\begingroup\$

Here is my list of recommendations:

  • To make the database object available in the custom functions' scope, pass it as a parameter. I know your global declaration is commonly used by WPers, but I consider it to be sloppy development. If you are going to persist with using global, then you should only do it once per custom function.

  • In bestanswers() to make your SQL easier to read, use newlines, indentation, spaces around operators, ALLCAPS mysql keywords, double-quoting the full string, and single-quoting the values in the string. *That said, for the most secure and consistent project, you should use prepared statements anytime you are supplying variables to your query.

    $results = $wpdb->get_results(
        "SELECT *
         FROM wp_comments
         LEFT JOIN wp_custom_scoring 
             ON wp_custom_scoring.entryID = wp_comments.comment_ID
         WHERE class = 'point'
             AND comment_post_ID = " . (int)$a . "
             AND type = 'plus'
         GROUP BY entryID
         ORDER BY COUNT(entryID) DESC");
    
  • empty() does 2 things. It checks if a variable !isset() OR loosely evaluates as "falsey". You know that $result will be set because you have unconditionally declared it on the previous line of code. (untested...)

    if (!$results)) {
        echo 'Its Empty!';
        return false;
    }else{
        return $results;
    }
    

    Or if you don't need the echo:

    return $results ? $results : false;
    
  • In scores(), your $b parameter only determines the WHERE clause, so you can DRY out your code like this:

    function score($wpdb, $a, $b) {
        if (in_array($b, ['plus','minus']) {
            $where = "class = 'point' AND type = '{$b}'";
        } else {
            $where = "class = 'fav'";
        }   
    
        $results = $wpdb->get_results(
            "SELECT COUNT(id) as total
             FROM wp_custom_scoring
             WHERE entryID = " . (int)$a . "
               AND {$where}";
    
        if (!$results || !$results[0]->total) {
            return 0;
        } elseif ($a === 'plus') {
            return "+{$results[0]->total}";
        } elseif ($a === 'minus') {
            return "-{$results[0]->total}";
        } else {
            return $results[0]->total;
        }
    }
    

    but again, I urge you to consider using prepared statements as a consistent/stable/secure technique across your whole application.


Ultimately, I don't know what is down in the ...etc. portion of your script, but if you are making two function calls to calculate: score($bests->comment_ID,"plus") + score($bests->comment_ID,"minus") > 0, then it may make better sense to create a single custom function that performs this calculation and returns the sum instead.

\$\endgroup\$
2
  • \$\begingroup\$ Very very good advices! Thank you so much! And yes, I guess I have to use $wpdb->prepare instead of $wpdb->get_results. But in this example intval() command must be securing my query against inj. am I wrong? ( I mean yes, $wpdb->prepare most secure way to protect a query, I'm talking for this code. Ofc I can't use intval() everywhere in my queries. It's only works with numbers...) And, etc means -> everything needed for my comment template. Like $bests->user_id, $bests->comment_ID, $bests->author_name... And you are totally right, I guess I'm calling so many queries for it. \$\endgroup\$ – techn9ne Aug 1 '19 at 1:49
  • \$\begingroup\$ Your posted queries are technically not vulnerable to injection attacks. \$\endgroup\$ – mickmackusa Aug 1 '19 at 1:51

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.