Dynamically-loading interactive table

Question 1

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.

Question 2

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

Question 3

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.

Question 4

@skiwi ah, I actually overlooked the call to mysql_real_escape_string - sorry (but note that that function has been deprecated)

Question 5

If $_GET['id'] is numeric, better cast it to an int instead of using mysql_real_escape_string.

Flambino Flambino 33.3k2 gold badges46 silver badges90 bronze badges · Accepted Answer · 2013-11-18 04:37:13Z

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

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.
@skiwi ah, I actually overlooked the call to mysql_real_escape_string - sorry (but note that that function has been deprecated)
If $_GET['id'] is numeric, better cast it to an int instead of using mysql_real_escape_string.

Stack Exchange Network

Dynamically-loading interactive table

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Dynamically-loading interactive table

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions