HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Code Reiview for an PHP PDO Queries? is there a better way to do it?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
reiviewphpwaybetterforcodequeriestherepdo

Problem

so this is my code for a equipping an item in my game dev't:

try {
        $db = getConnection();

        $db->beginTransaction();

        $sql_chara_gold = $db->query("SELECT chara_gold FROM chara WHERE chara_id = $shop->chara_id");
        $get_chara_gold = $sql_chara_gold->fetchColumn();

        $sql_shop_price = $db->query("SELECT item_price FROM shop WHERE item_id = $shop->item_id");
        $get_shop_price = $sql_shop_price->fetchColumn();

        if($get_chara_gold >= $get_shop_price){
            $sql_put_item = "INSERT INTO bag(chara_id, item_id, item_qty)
            VALUES(:id,:item_id,1) 
            ON DUPLICATE KEY 
            UPDATE item_qty = item_qty +1";

            $sql_set_gold = "UPDATE chara ch 
            INNER JOIN item it
            ON it.item_id = :item_id
            SET ch.chara_gold = ch.chara_gold - it.item_price 
            WHERE chara_id = :id";

            $sql_set_qty =  "UPDATE shop 
            SET item_qty = item_qty - 1 
            WHERE item_id = :item_id";

            $stmt = $db->prepare($sql_put_item);  
            $stmt->bindParam("id", $shop->chara_id);
            $stmt->bindParam("item_id", $shop->item_id);
            $stmt->execute();

            $stmt = $db->prepare($sql_set_gold);  
            $stmt->bindParam("id", $shop->chara_id);
            $stmt->bindParam("item_id", $shop->item_id);
            $stmt->execute();

            $stmt = $db->prepare($sql_set_qty);  
            $stmt->bindParam("item_id", $shop->item_id);
            $stmt->execute();

            $db->commit();

            $db = null;

            echo json_encode($shop); 
        }else{
            echo '{"error":{"text":'.json_encode($get_shop_price).'}}'; 
        }

Solution

I don't know if I'd use ORM for something so simple, but it may be something to look into. At the very least, you should get in the practice of escaping query parameters to avoid SQL injection. I often use Aura SQL for this, because it's dead simple: https://github.com/auraphp/Aura.Sql

An example query using Aura would look like this:

$user = $db->fetchOne('SELECT chara_gold FROM chara WHERE chara_id = :id', [
    'id' => $shop->chara_id
]);


Aura will automatically escape any bound parameters, like :id, so you don't have to worry about SQL injection. If you don't use Aura, at least use PDO::quote.

Funds transfer is the textbook example of why you need transactions to meet the ACID requirements, so it's good that you're using one, but I don't see your rollback. I assume this is in your catch clause which wasn't included.

Code Snippets

$user = $db->fetchOne('SELECT chara_gold FROM chara WHERE chara_id = :id', [
    'id' => $shop->chara_id
]);

Context

StackExchange Code Review Q#25861, answer score: 2

Revisions (0)

No revisions yet.