3
\$\begingroup\$

This is an auction function for a game where you can bid on houses on the website.

You set a limit, and if posts limit below current bet or below your limit, it will raise the bid from current plus 1, until the limit is met. If your bid is over a player's limit, it updates it with your limit.

Let's say the current limit for house #55 is 500 and was submitted by player CykaBlyat and then Andrej comes and submit a limit of 800 it will update latest limit plus one (so bid will be 501) and keep raising with 1 until someone submit a limit higher than 800. And so on.

Is it safe? Is there something I have missed? Something I can do better? A cleaner way?

$id = isset($_GET['id']) ? intval($_GET['id']) : 0;

// Fetch some info about the house
$house_query = "SELECT h.id AS houseID, h.name AS houseName, h.rent, h.size, h.numofdoors, h.beds, h.owner, h.paid, p.name
            FROM houses h
            LEFT JOIN players p ON p.id = h.owner
            WHERE h.id = ?";

$stmt = $db->prepare($house_query);
$stmt->execute(array($id));
$row = $stmt->fetch();

// Is house on auction
$auctions = "SELECT * 
            FROM house_auctions ha
            LEFT JOIN players p ON p.id = ha.player_id
            WHERE ha.house_id = $id";

$stmt = $db->prepare($auctions);
$stmt->execute(array($id));
$auction = $stmt->fetch();

if (count($_POST) > 0) {

$limit = trim($_POST['limit']);
$char = trim($_POST['character']);

// Get balance
$balance = $db->prepare("SELECT balance FROM players WHERE id = ?");
$balance->execute(array($char));

if ($limit <= $auction['bid']) 
    $errors[] = "Your limit for the bid is lower than the current bid.";


else if ($limit == $auction['bid']) 
    $errors[] = "Limit is same as current bid.";

// Check their balance
else if ($limit + $row['rent'] > $balance->fetchColumn()) 
    $errors[] = "You do not have enough gold.";

else if ($limit < $auction['limit']) { 
    $bid = $limit + 1;
    $limit2 = $auction['limit'];
    $pid = $auction['player_id'];
}
else if ($limit > $auction['limit']) { 
    $bid = $auction['limit'] + 1;
    $limit2 = $limit;
    $pid = $char;
} else { 
   $bid = $auction['limit'] + $limit + 1;
   $limit2 = $limit;
   $pid = $char;
}
// Let's update!
if (empty($errors)) {
    echo "NO ERRORS =)";

    $stmt = $db->prepare("UPDATE house_auctions SET house_id = :house_id, player_id = :player_id, bid = :bid, `limit` = :limit, endtime = :endtime");
    $stmt->execute(array(':house_id'=>$id, ':player_id'=>$pid, ':bid'=>$bid, ':limit'=>$limit2,':endtime'=>time()));

}
}

// If there were any errors, show them
if (!empty($errors)) {
foreach ($errors as $cur_error)
echo '<div class="padded box red" style="margin-bottom: 10px;">'.$cur_error.'</div>';
}
\$\endgroup\$
1
\$\begingroup\$

There's a significant security issue in this code. Note that you are getting the player ID from user input. What if player 100 submits a bid for player 200?

This might be safe ($id is forced to be an int previously) but is definitely squirrelly:

// Is house on auction
$auctions = "SELECT * 
            FROM house_auctions ha
            LEFT JOIN players p ON p.id = ha.player_id
            WHERE ha.house_id = $id";

$stmt = $db->prepare($auctions);
$stmt->execute(array($id));
$auction = $stmt->fetch();

You are embedding the value in the SQL. You should replace the $id in the query with a ?. Also, I'd use a heredoc or nowdoc to write the query rather than relying on a multiline double-quote string.

// Is house on auction
$auctions = <<<'EOSQL'
SELECT * 
FROM house_auctions ha
  LEFT JOIN players p ON p.id = ha.player_id
WHERE ha.house_id = ?
EOSQL;

$stmt = $db->prepare($auctions);
$stmt->execute(array($id));
$auction = $stmt->fetch();

I also indented the LEFT JOIN to indicate that it is part of the FROM clause.

else if ($limit == $auction['bid']) 
    $errors[] = "Limit is same as current bid.";

// Check their balance
else if ($limit + $row['rent'] > $balance->fetchColumn()) 
    $errors[] = "You do not have enough gold.";

I personally avoid using the single statement form of if. I always create a curly brace block. This helps if you need to add additional statements. This is especially true because in the same set of else if clauses you later do use the block version.

I would never put either whitespace or a comment between else if blocks. This makes it look like your else if structure is finished, so the next person may read it wrong. This is another advantage of the block structure, as you can put either or both in the block.

else if ($limit == $auction['bid']) {
    $errors[] = "Limit is same as current bid.";
}
else if ($limit + $row['rent'] > $balance->fetchColumn()) {
    // If their balance is insufficient...
    $errors[] = "You do not have enough gold.";
}

These errors are confusing to me. The limit is the same as the current bid, so I should ... Increase the bid? Decrease the limit? What? As a user, I'd appreciate some direction.

The name $row has lost me. I have to go back to find out what set $row. Even something like $house_row would be easier to remember.

Everywhere else, you process down to simple data structure (an associative array). Why do $balance differently? You could easily do

$player_balance = $balance->fetchColumn();

or

$player = $balance->fetch();

and later use either $player_balance or $player['balance'] instead of $balance->fetchColumn(). The way that you do it now, it would be very easy to add a second column to your balance query and break the existing code. If you immediately move the result from a database variable to a simple variable (either an array or a scalar), then all the database code is together. If you make a change that requires another change, then at least the code is right there.

I would actually prefer the $player['balance'] method, as it is more rigorous in the face of changing code. If you always fetch the array, then you don't have to worry about changing the dependent code if you add one or more columns to the query. It stays the same.

} else { 
    $bid = $auction['limit'] + $limit + 1;
    $limit2 = $limit;
    $pid = $char;
}
// Let's update!
if (empty($errors)) {
    echo "NO ERRORS =)";

Here you fully cuddle your } else { while elsewhere you put the } on a separate line. Try to format consistently so as to make your code predictable. Personally, I prefer the fully cuddled version, but either way can work if used consistently.

Try to avoid numbered variables. Something like $new_limit is preferable to $limit2 as it gives some indication of what causes the two to differ.

You use both $bid and $pid in that block. This is confusing, as I think that $pid is an abbreviation for $player_id while $bid isn't an abbreviation for anything. It's often better to avoid abbreviations, as they slow down reading. You will find that you spend more time reading code than writing it in most applications, so it's better to spend a little extra time writing now than waste time reading later.

You don't leave any whitespace after the end of the else block and before the comment before the next statement. Previously I suggested that you not use whitespace because it indicated separation where none existed. Here, there is separation, so go ahead and use whitespace to indicate it.

Your comment says, "Let's update!" But what you actually do next is check for errors. I would have put that comment inside the block for that reason. Although in this case, I don't find the comment necessary. The UPDATE in the SQL is obvious enough so that we don't need an additional notice.

Rather than doing an echo, you would normally redirect at this point to clear out the POST (which helps avoid accidentally repeating the action). You can redirect to a success page.

Typically the time when you want to comment is to explain why you are doing something. For example, why does the player's balance need to be greater than the sum of the limit and the rent? Is that a game rule? It's not an obvious one.

Why are doing all these else if statements at all? These seem to be disjunct. You even have the capability of handling multiple errors, but you never generate more than one at a time.

// If there were any errors, show them
if (!empty($errors)) {
foreach ($errors as $cur_error)
echo '<div class="padded box red" style="margin-bottom: 10px;">'.$cur_error.'</div>';
}

Again, your comment tells me something I already know.

This block is confusingly indented. It's hard to tell what goes with what.

Your if doesn't guarantee that it won't crash on the foreach. The foreach expects $errors to be an array, but you don't check for that. If $errors is any other kind of non-empty variable (quite possible since it is never initialized in this block), it will pass the if but can still blow up the foreach. The foreach can handle an empty array, but it will blow up on an unset variable or a non-array.

Why bother with $cur_error? $error would seem sufficient. If there is something especially current about it, why not say $current_error instead?

It can be hard to find styles if they can appear in both code and CSS. For that reason, I would tend to put the margin-bottom: 10px; in a CSS class somewhere. It's also wrong to name CSS classes things like padded, box, or red. The HTML should tell you what it is, not how it should look. So the class should be something like error_message.

\$\endgroup\$
2
\$\begingroup\$

Use placeholder there: WHERE ha.house_id = $id"; Placeholder is better due to security approach.


if (count($_POST) > 0) {

$limit = trim($_POST['limit']);
$char = trim($_POST['character']);

You should separate presentation from your logic. It will be better: read all $_POST outside the function and pass in arguments. Maybe you wanna later to play your game via AJAX, or something else, and you won't need $_POST anymore, but you still need the logic.

Due the same approach, returning status and errors from function is a better way, then just prints them out. You can return 2 values from function -- status and errors array.

Then do something like this:

list($status, $errors) = your_function(...);
if (!$status) {
    //printing errors
}

Or you can just return an array, and check length of it. If array contain some errors, print them out.


Rename your variables.

char to character

pid to playerId

limit2 to newLimit? Or what it means?

In your code it isn't clear enough. Especially limit2.

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