CODEWITHSUNDEEP
phpsql
Orel Bitton
Orel Bitton

Reputation: 597

Prepared statements, return before closing statement

I have this function here, which globals the $con variable which is the mysql connection variable, topic name is taken by a paramater as you can see. i have created a variable called count for counting the number of rows returned by the statement.

My question is, is using this ok? or there is a better way of doing that?

function isTopic($topic_name){
    global $con;

    $topic_name=$con->real_escape_string($topic_name);
    $count = 0;
    if($stmt = $con->prepare("SELECT topic_id FROM topics WHERE topic_name = ?")){
        $stmt->bind_param("s",$topic_name);
        $stmt->execute();
        $stmt->store_result();
        $count = $stmt->num_rows;
        $stmt->close();
    }
    return ($count == 1);
}

Upvotes: 0

Views: 113

Answers (3)

samayo
samayo

Reputation: 16495

It is very wrong, because you do not need to use real_escape_string() when you are using prepared statements It defeates the purpose of of using prepared statements.

Second, as noted in the comment section, using global for getting the resources object is not encouraged, for lots of reason. Instead, you can pass the object as argument for your function, if you are working in a procedural style, otherwise, if you are codding in OO style, then you can apply DI(Dependency Injection) and get the resources.

I will assume, you are not using OOP and provide a procedural answer here.

If you are tying to returns row count just do.

function isTopic($conn, $topic_name){
    $stmt = $conn->prepare("SELECT topic_id FROM topics WHERE topic_name = ?");
                $stmt->bind_param("s", $topic_name);
                $stmt->execute();
         return $stmt->rowCount();
}

Upvotes: 2

CodeAngry
CodeAngry

Reputation: 12995

WHY WOULD YOU RETURN ACTUAL ROWS OF DATA JUST TO COUNT THEM?

When MySQL can do this for you:

// if you have uniques
SELECT COUNT(`topic_id`) FROM `topics` WHERE `topic_name` = ?;
// if you have duplicates
SELECT COUNT(DISTINCT `topic_id`) FROM `topics` WHERE `topic_name` = ?;

It's so easy and MySQL returns a number not records, that's one integer vs. an array of data structures.

If you need a count use COUNT().

PS: String escaping with prepared statements = redundancy + fail.

UPDATE:

If you just need to check for existence of that topic_name:

SELECT `topic_id` FROM `topics` WHERE `topic_name` = ? LIMIT 1; // important!

Just select the topic_id and limit to 1 match. Still 1 integer return.

Upvotes: 3

Your Common Sense
Your Common Sense

Reputation: 157919

Your code is quite all right. However, there is too much useless code to my taste.

If I were you, I'd make it at least

function isTopic($topic_name){
    global $con;

    $stmt = $con->prepare("SELECT topic_id FROM topics WHERE topic_name = ?");
    $stmt->bind_param("s", $topic_name);
    $stmt->execute();
    $stmt->store_result();
    return $stmt->num_rows;
}

But if I were myself I'd make it 

function isTopic($topic_name){
    global $con;
    $sql = "SELECT topic_id FROM topics WHERE topic_name = ?"
    return $con->getOne($sql, $topic_name);
}

Upvotes: 0

Related Questions