How to improve this line of code getting the json and put it to a table using jquery and php

Question

jquery code:

$.ajax({
                    url: "get_attributes.php",
                    type: "post",
                    datatype: "json",
                    data: {wtype: red_type},
                    success: function(data) {
                        var toAppend = '';
                        if(typeof data === "object"){
                            toAppend += "<tbody>";
                            toAppend += "<tr></td><td class=datalabel>Type:</td><td>"+data['type']+"</td></tr>";
                            toAppend += "<tr><td class=datalabel>Health:</td><td>"+data['health']+"</td></tr>";
                            toAppend += "<tr><td class=datalabel>Attack:</td><td>"+data['attack']+"</td></tr>";
                            toAppend += "<tr><td class=datalabel>Defense:</td><td>"+data['defense']+"</td></tr>";
                            toAppend += "<tr><td class=datalabel>Speed:</td><td>"+data['speed']+"</td></tr>";
                            toAppend += "<tr><td class=datalabel>Evade:</td><td>"+data['evade']+"</td></tr>";
                            toAppend += "<tr><td class=datalabel>Special:</td><td>"+data['special']+"</td></tr>";
                            toAppend += "</tbody>";
                            $("tbody").remove();
                            $("#red_form table").append(toAppend);
                        }
                    }
                });

php code:

    <?php
header("Content-Type: application/json");

$wtype = $_POST['wtype'];

$attributes = array();

if($wtype == 'Ninja'){
    $attributes['type'] = 'Ninja';
    $attributes['health'] = '40-60';
    $attributes['attack'] = '60-70';
    $attributes['defense'] = '20-30';
    $attributes['speed'] = '90-100';
    $attributes['evade'] = '0.3-0.5';
    $attributes['special'] = 'There is a 5% chance of 2x attack';
}
else if($wtype == 'Samurai'){
    $attributes['type'] = 'Samurai';
    $attributes['health'] = '60-100';
    $attributes['attack'] = '75-80';
    $attributes['defense'] = '35-40';
    $attributes['speed'] = '60-80';
    $attributes['evade'] = '0.3-0.4';
    $attributes['special'] = '10% chance of restoring +10 health when evade is successful';
}
else if($wtype == 'Brawler'){
    $attributes['type'] = 'Brawler';
    $attributes['health'] = '90-100';
    $attributes['attack'] = '65-75';
    $attributes['defense'] = '40-50';
    $attributes['speed'] = '40-65';
    $attributes['evade'] = '0.3-0.35';
    $attributes['special'] = 'increased +10 defense when health is below 20%';
}

echo json_encode($attributes);
?>

Answer 1

There are a few improvements that come to my mind for the given code:

1. you may want to return an status code for your JSON output instead of checking whether the result is an object. It would allow you to handle error messages and such in a more convenient manner.

2. as a minor improvement, you can use replaceWith() to replace the table body instead of removing it and then appending a new one to the table.

3. you don't have an error handler to your script... if you won't ever need it, that's cool, but it's always best to know when a script fails :)

4. to improve concatenation performance, you might want to use Array's join() method instead of doing += for each table row.

5. you're now removing all table bodies on the whole page, which could be kind of risky if you have more than one table there. Furthermore, the selector is extensively greedy, as jQuery has to find all table bodies on the whole page. It's better to use ID selectors when possible, as they are the fastest one to execute.

6. PHP-wise, you don't handle invalid $wtype, so if no condition is met, you won't know what went wrong, since nothing relevant will be returned

Enough of the theory, this is what I have in mind:

$.ajax({
                url: "get_attributes.php",
                type: "post",
                datatype: "json",
                data: {wtype: red_type},
                success: function(data) {
                    if (data["status"] == 200) {
                        var toAppend = [];
                        toAppend.push(
                            "<tbody>",
                            "<tr></td><td class=datalabel>Type:</td><td>"+data['type']+"</td></tr>",
                            "<tr><td class=datalabel>Health:</td><td>"+data['health']+"</td></tr>",
                            "<tr><td class=datalabel>Attack:</td><td>"+data['attack']+"</td></tr>",
                            "<tr><td class=datalabel>Defense:</td><td>"+data['defense']+"</td></tr>",
                            "<tr><td class=datalabel>Speed:</td><td>"+data['speed']+"</td></tr>",
                            "<tr><td class=datalabel>Evade:</td><td>"+data['evade']+"</td></tr>",
                            "<tr><td class=datalabel>Special:</td><td>"+data['special']+"</td></tr>",
                            "</tbody>"
                        );
                        $("#red_form table tbody").replaceWith(toAppend.join(""));
                    } else {
                        alert('An error has occured. Please try again.');
                        console.log('Status: ' + data["status"] + ', message: ' + data["message"]);
                    }
                },
                error: function(jqXHR, textStatus, errorThrown) {
                    alert('An error has occured. Please try again.');
                    console.log('ErrStatus: ' + textStatus + ', error: ' + errorThrown);
                }
            });

And the PHP:

<?php
header("Content-Type: application/json");
$wtype = (isset($_POST['wtype']) ? $_POST['wtype'] : 'unknown');
$attributes = array('status' => 200, 'message' => 'ok', 'type' => $wtype);

switch ($wtype) {
    case 'Ninja':
        $attributes['health'] = '40-60';
        $attributes['attack'] = '60-70';
        $attributes['defense'] = '20-30';
        $attributes['speed'] = '90-100';
        $attributes['evade'] = '0.3-0.5';
        $attributes['special'] = 'There is a 5% chance of 2x attack';
        break;

    case 'Samurai':
        $attributes['health'] = '60-100';
        $attributes['attack'] = '75-80';
        $attributes['defense'] = '35-40';
        $attributes['speed'] = '60-80';
        $attributes['evade'] = '0.3-0.4';
        $attributes['special'] = '10% chance of restoring +10 health when evade is successful';
        break;

    case 'Brawler':
        $attributes['health'] = '90-100';
        $attributes['attack'] = '65-75';
        $attributes['defense'] = '40-50';
        $attributes['speed'] = '40-65';
        $attributes['evade'] = '0.3-0.35';
        $attributes['special'] = 'increased +10 defense when health is below 20%';
        break;

    default: $attributes['status'] = 400;
             $attributes['message'] = 'Invalid class info has been requested.';
}

echo json_encode($attributes);
?>

Answer 2

+1 To Zathrus, an excellent review. Just wanted to add a few of things about the PHP. Can't really help you with the jquery.

First, to clarify why he is using a switch here instead of if/elseif statements. This because they are a bit cleaner and a bit faster. The first I'm sure you are able to see from his implementation, but the second won't be as noticeable without profiling.

If we follow the DRY principle, that I mentioned from my answer to your last question, then we know only to set the type once since it is a repeated task that can easily be abstracted.

//after switch
$attributes[ 'type' ] = $wtype;

But we can take that a step further. Even though its hard to see how we can fix it, the list of attributes also violates the DRY principle. Since all you are doing is creating a JSON object to return to the web page, why not just start off with one from the very beginning? Create a JSON file for every "type" of warrior and then you can do everything with just a single line:

return file_get_contents( "path/to/json/$wtype.json" );

You could also change this so that its just one JSON file for all warriors and you select which type after loading the file, but the above allows for the immediate return of a JSON object.

But, as Zathrus mentioned, you might also want to return the status. So this is just to give you some ideas.

Answer 3

How about this? I moved the repeated code in $attributes to its own function. Also, +1 to Zathrus' review as well as mseancole's additions. Predefined json files might not be a bad idea.

header("Content-Type: application/json");
$wtype = (isset($_POST['wtype']) ? $_POST['wtype'] : 'unknown');

/**
 * Set attributes for character type
 * 
 * return array
 */
function setAttributes($health, $attack, $defense, $speed, $evade, $special, $wtype){
    $attributes['health'] = $health;
    $attributes['attack'] = $attack;
    $attributes['defense'] = $defense;
    $attributes['speed'] = $speed;
    $attributes['evade'] = $evade;
    $attributes['special'] = $special;
    $attributes['status'] = 200;
    $attributes['message'] = 'ok';
    $attributes['type'] = $wtype;
    return $attributes;
}

switch ($wtype) {
    case 'Ninja'  : $attributes = setAttributes('40-60', '60-70', '20-30', '90-100', '0.3-0.5', $wtype, 
                                   'There is a 5% chance of 2x attack'); break;

    case 'Samurai': $attributes = setAttributes('60-100', '75-80', '35-40', '60-80', '0.3-0.4', $wtype, 
                                   '10% chance of restoring +10 health when evade is successful'); break;

    case 'Brawler': $attributes = setAttributes('90-100', '65-75', '40-50', '40-65', '0.3-0.35', $wtype, 
                                   'increased +10 defense when health is below 20%'); break;

    default: $attributes['status'] = 400;
             $attributes['message'] = 'Invalid class info has been requested.';
}

echo json_encode($attributes);

And here's the code with mseancole's method of using predefined json files:

header("Content-Type: application/json");
$wtype = (isset($_POST['wtype']) ? $_POST['wtype'] : 'unknown');

switch ($wtype) {
    case 'unknown': {
                $attributes['status'] = 400;
                $attributes['message'] = 'Invalid class info has been requested.';
                return json_encode($attributes);
             }
    default return file_get_contents( "path/to/json/" . strtolower($wtype) . ".json" );
}