3
\$\begingroup\$

I've got a page on a website that shows a table, and upon clicking on a row in the table, it can dynamically load in more results. I am new to jQuery though.

index.php page:

<!DOCTYPE html>
<html>
    <head>
        <title>Knife kills</title>
        <script src="jquery-2.0.3.min.js"></script>
        <script type="text/javascript">
            $(document).ready(function() {
                $("#list tr").bind("click", function(event) {
                    //Get clicked row
                    var clickedRow = $("#list tr#" + this.id);

                    //Increase clicks
                    if (!$(clickedRow).data("clicks")) {
                        $(clickedRow).data("clicks", 0);
                    }
                    $(clickedRow).data("clicks", $(clickedRow).data("clicks") + 1);

                    //Check if detailed information has been retrieved
                    if ($(clickedRow).data("detailed")) {
                        if ($(clickedRow).data("clicks") % 2 == 0) {
                            //Hide
                            $("#list tr#" + this.id + "row").remove();
                        }
                        else {
                            //Show
                            $($(clickedRow).data("detailed")).insertAfter($(clickedRow));
                        }
                    }
                    else {
                        var loading = $("<tr><td colspan=3>Loading...</td></tr>");
                        loading.insertAfter($(clickedRow));
                        $.ajax({
                            url: "ajax_detailed.php",
                            data: {
                                id: this.id
                            },
                            success: function(html) {
                                if (html) {
                                    //Success
                                    $(loading).remove();
                                    $(html).insertAfter($(clickedRow));
                                    $(clickedRow).data("detailed", html);
                                }
                                else {
                                    //Error
                                    $(loading).remove();
                                }
                            }
                        });
                    }
                });
            });
        </script>
    </head>
    <body>
        <table id="list">
            <tr><th>#</th><th>Player</th><th>Kills</th><th></th><th></th></tr>
<?php
$con = mysql_connect("localhost", "bf4", "bf4") or die(mysql_error());
mysql_select_db("bf4") or die(mysql_error());
$query = mysql_query("SELECT p.playerId AS pid, p.playerName AS name, COUNT(`playerkills`.`id`) AS amount FROM `playerkills` JOIN players p ON playerkills.playerId = p.playerId WHERE weaponId = 9 GROUP BY playerkills.playerId ORDER BY amount DESC;") or die(mysql_error());
$i = 0;
$lastamount = -1;
$offset = 0;
while ($result = mysql_fetch_object($query)) {
    if ($lastamount != $result->amount) {
        $i += 1 + $offset;
        $offset = 0;
    }
    else {
        $offset++;
    }
    echo "\t\t\t<tr id='{$result->pid}' style='cursor: pointer;'><td>".$i."</td><td>".$result->name."</td><td>".$result->amount."</td><td></td><td></td></tr>\n";
    $lastamount = $result->amount;
}
mysql_close($con) or die(mysql_error());
?>
        </table>
    </body>
</html>

ajax_detailed.php page:

<?php
$con = mysql_connect("localhost", "bf4", "bf4") or die(mysql_error());
mysql_select_db("bf4") or die(mysql_error());

$id = mysql_real_escape_string($_GET['id']);

echo "<tr id='{$id}row'><th></th><th></th><th></th><th>Target</th><th>Kills</th></tr>";
$query = mysql_query("SELECT p.playerName AS name, COUNT(`playerkills`.`id`) AS amount FROM `playerkills` JOIN players p ON playerkills.targetId = p.playerId WHERE weaponId = 9 AND playerkills.playerId = '{$id}' GROUP BY playerkills.targetId ORDER BY amount DESC;");
while ($result = mysql_fetch_object($query)) {
    echo "<tr id='{$id}row'><td></td><td></td><td></td><td>".$result->name."</td><td>".$result->amount."</td></tr>";
}

mysql_close($con) or die(mysql_error());
?>

I have the feeling that my jQuery is way too complicated. I hope others can shed light on it.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Some basic things (looking only the at the JS)

  • Don't keep repeating $(clickedRow) - clickedRow is already a jQuery object. There's no need to use $() again.
  • Avoid manually tracking a number to figure out whether to hide or show the details. Easier to check if the details are present or not and proceed accordingly.

Functionality-wise, there are a lot of ways to do what you need. I'd suggest breaking things out into functions, and a lot of other stuff.

But I don't want to go too far from your current code, so here's a simple, "light" refactoring (and here's a demo)

$(function() { // same as $(document).ready()
  $("#list tr").on("click", function (event) { // use .on() instead of .bind() (since jQuery 1.7)
    // Get relevant rows, the simpler way
    var xhr,
        id = this.id,
        clickedRow = $(this),
        loadingRow = clickedRow.next(".loading"),
        detailsRow = clickedRow.next("#" + id + "row");

    if(loadingRow.length > 0) {
      return; // stop here if we're currently loading
    }

    // Hide details and stop, if the details are shown
    if(detailsRow.length > 0) {
      detailsRow.remove();
      return;
    }

    // insert cached details and stop, if possible
    if(clickedRow.data("details")) {
      clickedRow.data("details").insertAfter(clickedRow);
      return;
    }

    // if we get this far, we'll need to load the details, so:

    // show the loading message
    loadingRow = $('<tr class="loading"><td colspan="3">Loading...</td></tr>').insertAfter(clickedRow);

    // fetch details (using the deferred/promise API)
    xhr = $.ajax({
      url: "ajax_detailed.php",
      data: { id: id },
      cache: true
    });

    // add details when they've loaded
    xhr.done(function (html) {
      // I'm skipping the if(html) check, since the server should
      // return an error code if there's an error. And if so, this
      // done-handler will never be called anyway
      detailsRow = $(html).insertAfter(clickedRow);
      clickedRow.data("details", detailsRow);
    });

    // remove the loading row regardless of how the ajax request went
    xhr.always(function (html) {
      loadingRow.remove();
    });
  });
});

The PHP also looks iffy to me, but it's been a long time since I had to work with PHP, so I won't comment.

I will say though, that you're extremely vulnerable to SQL injection attacks. For instance, if I send an id value of 0'; DROP DATABASE bf4; DROP USER bf4; to your ajax_detailed.php script, it'll permanently delete your entire database and remove the database user. You may want to look into that...

Ignore the above - I had overlooked the call to mysql_real_escape_string

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ Okay I'll definitely need to refactor my code more into functions, thanks for the comments. As for SQL injection, the mysql_real_escape_string will cover that. However the code is definitely not 100% secure. \$\endgroup\$
    – skiwi
    Nov 18, 2013 at 9:08
  • \$\begingroup\$ @skiwi ah, I actually overlooked the call to mysql_real_escape_string - sorry (but note that that function has been deprecated) \$\endgroup\$
    – Flambino
    Nov 18, 2013 at 13:03
  • 1
    \$\begingroup\$ If $_GET['id'] is numeric, better cast it to an int instead of using mysql_real_escape_string. \$\endgroup\$
    – MarcDefiant
    Nov 18, 2013 at 13:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.