3
\$\begingroup\$

How can I improve this code?

jQuery:

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

    <?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);
?>
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

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);
?>
\$\endgroup\$
5
  • \$\begingroup\$ whats with the status? 200? 400? Thanks! this is this applicable right if my php example get the array values through foreach right? \$\endgroup\$ Sep 20, 2012 at 8:15
  • \$\begingroup\$ status is just a variable telling jQuery (in the success function) whether or not everything was processed ok... I've updated the code to use data["status"] notation which is probably closer to your coding style... if for instance someone will fake the $wtype, they'll get an alert saying that they picked an invalid character class \$\endgroup\$ Sep 20, 2012 at 9:05
  • \$\begingroup\$ also, I don't understand your other part of question about foreach \$\endgroup\$ Sep 20, 2012 at 9:05
  • \$\begingroup\$ example: $array = array(); foreach($stmt as $values){ $array[] = $values; } echo json_encode($array); Can I still implement the same of jquery code of yours? Thanks \$\endgroup\$ Sep 21, 2012 at 4:18
  • 1
    \$\begingroup\$ thanks for the clarification... you certainly can reuse the login with a foreach() statement, however the switch portion of the code might not be usable there in this form... it really depends on your user case \$\endgroup\$ Sep 21, 2012 at 6:51
3
\$\begingroup\$

+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.

\$\endgroup\$
2
  • \$\begingroup\$ Hi, thank you. I still not familiar with json how can I able to implement this? thanks! :) \$\endgroup\$ Sep 21, 2012 at 4:15
  • \$\begingroup\$ @brainedwashed: Try outputting json_encode on one of your current arrays to get an idea of what it should look like, then save that string to a json file. \$\endgroup\$
    – mseancole
    Sep 21, 2012 at 12:42
2
\$\begingroup\$

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" );
}
\$\endgroup\$

Your Answer

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

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