Update game content via AJAX

Question

On my application, I have a JavaScript cycle that updates tag contents on my page regularly:

<script type="text/javascript">
 var players = [];
 players.push($('#p0'));
 players.push($('#p1'));
 players.push($('#p2'));
 players.push($('#p3'));
 i=0;
 /*
 * cycle through the players.
 */
 function cyclePlayer() {
 players[i].parents(".card-header").addClass('border-success');
 players[i].load('play.php?p='+i);
 i = ++i % players.length;
 $('#deck').load('play.php?deck=1');
 $('#pile').load('play.php?pile=1');
 $('#feedback').load('play.php?feedback=1');
 }
 PlayerLoop = setInterval('cyclePlayer()', 1500 );
 $("#stop").click(function(){
 clearInterval(PlayerLoop);
 });
 $('#reset').click(function() {
 $('#deck').load('play.php?reset=1');
 location.reload();
 });
</script>

The server code that is being called looks like this:

<?php
require '../vendor/autoload.php';
session_start();
use Svc\Myapp\FrontHandler;
$myApp = new FrontHandler();
if (isset($_GET['p']) && !isset($_SESSION['winner'])) {
 echo $myApp->playerTurn();
}
if (isset($_GET['deck'])) {
 echo $myApp->deckUpdate();
}
if (isset($_GET['pile'])) {
 echo $myApp->pileUpdate();
}
if (isset($_GET['feedback'])) {
 echo $myApp->feedbackUpdate();
}

Is this production quality code? Is there a way to refactor it to improve it?

• FreezePhoenix: Considering your <?php tag never closes, I would say no.
• Terix: @FreezePhoenix Not closing the PHP tag is actually good practice, if the file has just PHP in it. stackoverflow.com/questions/3219383/…
• chicks: This still doesn't fit the site's guidelines. It isn't clear what you're trying to do with the code. And the title should be more descriptive and less prescriptive. codereview.stackexchange.com/help/how-to-ask
• jrswgtr: I am only commenting on style now. Variables should be defined with a keyword like var, const or let. They should start with a lowercase character. PlayerLoop = setInterval('cyclePlayer()', 1500 ); is a nono. Define it with var or let like this let playerLoop = setInterval('cyclePlayer()', 1500 );.

Answer 1

Louys already suggested simplifying the multiple requests into a single request - that is a great idea. I see a few other simplifications and suggestions.

The first few lines of the Javascript code push jQuery objects into an array:

var players = [];
players.push($('#p0'));
players.push($('#p1'));
players.push($('#p2'));
players.push($('#p3'));

While it isn't the same, one simplification would be to combine those selectors into a single selector:

var players = $('#p0, #p1, #p2, #p3');

Instead of an array of jQuery objects, that would be a single jQuery object with the four elements, and thus access to each element could be utilized via the .eq() method - e.g.

players.eq(i).parents(".card-header").addClass('border-success');
players.eq(i).load('play.php?p='+i);

Another consideration is to use a class name - for example, if those four elements all have the same class attribute, then that class name could be used in the selector.

---

As Louy's answer suggests, it would be wise to handle the case when the AJAX requests fail - for an example, see the last example in the documentation for .load().

---

Also, as Louy's answer implied, it would be wise to use json_encode() to send data in JSON format from the PHP code, and then parse it in the AJAX success callback - even if there was a single set of data.

---

The code for the FrontHandler class was not included but we see that it has methods like playerTurn, deckUpdate, etc. Do you consider that class a controller? Perhaps it could have a single method to return the appropriate data based on the supplied parameters.

Answer 2

My answer focuses on the ajax requests.

Why 4 requests at each 1500 milliseconds to the same file (play.php)?

I would do only one request and I would "cycle" on a delay starting from the success callback... instead of a fixed interval, which does not take the request delay in account.

var players = [ // No need to use a push method when you already know the array content.
 $('#p0'),
 $('#p1'),
 $('#p2'),
 $('#p3')
];
i=0;
function cyclePlayer() {
 $.ajax({
 url: "play.php",
 data: {iteration:i},
 method: "GET",
 dataType: "json",
 success: (data)=>{
 console.log(JSON.stringify(data)); // See all the infos in the same request response.
 json = JSON.parse(data);
 players[i].html(json.playerTurn).parents(".card-header").addClass('border-success');
 i = ++i % players.length;
 $('#deck').html(json.deckUpdate);
 $('#pile').html(json.pileUpdate);
 $('#feedback').html(json.feedbackUpdate);
 PlayerLoop = setTimeout(cyclePlayer,1500); // From here, set a new 1500ms timeout.
 },
 error: ()=>{
 },
 });
}
// on load, start a timeout.
var PlayerLoop = setTimeout(cyclePlayer, 1500 );
$("#stop").click(function(){
 clearTimeout(PlayerLoop);
});
$('#reset').click(function() {
 location.reload(true); // true is to reload from server instead of possibly loading from cache.
});

And the PHP:

<?php
require '../vendor/autoload.php';
session_start();
use Svc\Myapp\FrontHandler;
if(!isset($_GET['iteration'])){die();}
$myApp= new FrontHandler();
if(!isset($_SESSION['winner'])){
 $results["playerTurn"] = $myApp->playerTurn();
}
$results["deckUpdate"] = $myApp->deckUpdate();
$results["pileUpdate"] = $myApp->pileUpdate();
$results["feedbackUpdate"] = $myApp->feedbackUpdate();
$results["isSetWinner"] = isset($_SESSION['winner']);
// Echo the array as a json. This will be the response of the request.
echo json_encode($result);
?>

Now, I could not test the above... I hope It does not have any typo.