Checking and displaying JSON data

Question

I mainly develop in PHP and every now and again I have to do stuff in JavaScript which I'm not really comfortable, so I would like a review of how I can improve my code and knowledge. This script takes data coming from JSON, does some basic checking on the data fro format and things like that and then displays the data.

The code also calls a function in PHP that deals with updating the data in the JSON file, and then repeats the process.

$(document).ready(
 function () {
 setInterval(function () {
 console.log("start");
 $.ajax({url: "/QcT/bulk"})
 .done(function (data) {
 console.log("Done");
 $.getJSON("../../../UpdatedData/QC/QC.json", function (data) {
 var div = document.getElementById('Store');
 div.innerHTML = "";
 var i = 0;
 for (var i = 0; i < data.length; i++) {
 var Machine = data[i];
 //Correctley format the machine name
 if (Machine.MachineNo < 10) {
 MachineName = "ZW0100" + Machine.MachineNo;
 } else {
 MachineName = "ZW010" + Machine.MachineNo;
 }
 // Test to see if we have a first off. 
 console.log("Machine" + MachineName + "override " + Machine.FirstoffOverride + " FirstOFfTime " + Machine.FirstoffTime);
 if (Machine.FirstoffTime !== null) {
 Firstoff = "<div class='col-md-1' style='border-color: black;padding-left: 0px;margin-right: 2px;border-style: solid;border-width: 2px;-webkit-border-radius: 27px;-moz-border-radius: 27px;border-radius: 27px;padding-right: 0px;background-color: greenyellow;font-size: 30px;text-align: center;width: 46px;margin-left: 10px;color:greenyellow;' > 1 </div>";
 } else {
 Firstoff = "<div class='col-md-1' style='visibility: hidden; padding-left: 0px;margin-right: 2px;border-style: solid;border-width: 2px;-webkit-border-radius: 27px;-moz-border-radius: 27px;border-radius: 27px;padding-right: 0px;background-color: ##DFDFDF;font-size: 30px;text-align: center;width: 46px;margin-left: 10px;color:#DFDFDF;' > 1 </div>";
 }
 //Test to see if a comment is set 
 if (Machine.comment !== null) {
 comment = Machine.comment;
 } else {
 comment = " ";
 }
 var color = "";
 var time = "";
 content = "<div class='panel panel-default' style='font-weight: 700;margin-bottom:0px;border-radius: 46px;background-color: #DFDFDF ;box-shadow: 3px 5px 14px #474747;height:63px;margin-bottom: 11px;' >";
 content += "<div class='panel-body' style='padding: 10px;'>";
 content += "<div class='col-md-8'>";
 content += "<div class='col-md-2' style='font-size: 33px;color:black'>" + MachineName + "</div>";
 content += Firstoff;
 content += "<div class='col-md-9 text-center' style='font-size: 28px;color:Black;'>" + comment + "</div></div>";
 content += "<div class='col-md-4'>";
 $.each(Machine.PassFail, function (key, value) {
 //Check to see if it's pass or fail
 if (value.password === '0001') {
 color = "greenyellow";
 time = value.ReaderTime;
 } else {
 color = "red";
 time = value.ReaderTime;
 }
 content += "<div class='col-md-1' style='padding-left:0px;margin-right: 2px;border-style: solid;border-width: 2px;-webkit-border-radius: 27px;-moz-border-radius: 27px;border-radius: 27px;padding-right:0px;background-color:" + color + ";font-size: 30px;text-align: center;' >" + time + "</div>";
 });
 content += "</div></div></div></div>";
 $(content).appendTo(div);
 }
 ;
 console.log("refresh");
 });
 });
 }, 6000);
 });

The data from the Ajax call runs the script that updates the JSON File. It doesn't directly interact with the data, but just updates the JSON file:

/../../UpdatedData/QC/QC.json

Answer 1

Here are some tips based on pure code-style, including some jQuery tips, personal preferences and JavaScript best practices I am aware of:

• don't use anonymous functions

Code like this $(document).ready(function () { /* ... */ }); or this setInterval(function () { /* ... */ }, delay); use anonymous functions. I prefer to use function declaration (or at least named function) to help during code debugging, and to help reuse functions. Also, in a general way, I find that using named functions help you architecture your code in a better way and avoid the spaghetti code. What about something like this:

function onDocReady () { setInterval(timerFired, delay); }
function timerFired () { /* ... */ }
$(document).ready(onDocReady);

An "hybrid" solution could have been this:

function timerFired () { /* ... */ }
$(document).ready(function onDocReady () { // notice that this function has a name
 setInterval(timerFired, delay);
});

I find this much more readable than the code below, and it avoids callback-hell code:

$(document).ready(function () {
 setInterval(function () {
 // ...
 }, delay);
});

You can read Angus Croll post about this: https://javascriptweblog.wordpress.com/2010/07/06/function-declarations-vs-function-expressions/

• don't use jQuery.ready when you don't need it

In most situations, jQuery ready event is not needed. I have explained it in another stackoverflow post so you can read it here. In your case, I assume you're in the situation "FOUR" and so you can almost safely switch to situation "THREE" and remove the jQuery.ready call.

• avoid setInterval

setInterval should be used with huge caution. Basically, setInterval ignores errors and network latency, so it might be harmful. Also, if the time of execution of the "intervalled" function is longer than the delay, your browser may have hard time handling this. The equivalent of this code setInterval(timerFired, delay); is the following, using safer setTimeout:

function timerFired () {
 /* do work */
 setTimeout(timerFired, delay);
}
timerFired();

You can read more about this here, with some demos: http://zetafleet.com/blog/why-i-consider-setinterval-harmful

• be careful with relative path

In this code $.getJSON("../../../UpdatedData/QC/QC.json", fn), be careful with relative path, maybe one day you'll want to change the location of your JavaScript file and it won't work anymore because of this. Maybe you can have all your "config" values like this grouped in the top of the file, something like this:

var myConfig = {
 urlBulk: '/QcT/bulk',
 urlQC: '../../../UpdatedData/QC/QC.json'
};
$.ajax({url: myConfig.urlBulk})
.done(function onBulkResultGot (data) {
 $.getJSON(myConfig.urlQC, onQCResultGot);
});

• Cache length in loops

The code for (var i = 0; i < data.length; i++) should be written like this for (var i = 0, len = data.length; i < len; i++). This avoid calculating the length property on each iteration loop. This advice does not apply in one situation although: when the length changes during the loop. But as far as I see, this is not your case here.

• Use consistent naming convention

In your code, we can find this var div = document.getElementById('Store'); and this var Machine = data[i];. Both variable div and Machine doesn't follow the same naming convention. You should pick one and stick with it. In general, JavaScript developers like to use camelCase or snake_case for variable name, and PascalCase for class and sometimes functions names.

• Be careful with variable declarations

Both Firstoff and content variables are not declared using var keyword. This can lead to some tricky bug so don't forget to use it. Also, your code can be read as follows:

$.ajax({url: '/some/url'}).done(function (data) {
 $.getJSON('/another/url', function (data) {
 });
});

You have declared two variable with the same name, data. Also, both color and time variables are not used: only a value is affected but not used anywhere else.

• Use better variable names

You have variable named data, div. What are they supposed to represent ? Those names are way too generic IMHO. What if you have to deal with a second div ? Will you name it div2 ? Try to find better name for them, which explains why they are variable, what do they refer too etc..

• jQuery.each may be overkill

In most cases, jQuery.each(element, fn) can be replace by a far more simple element.forEach(fn). This is true if element is not null and is a true array (not an object). So your code could be written as follow: Machine.PassFail.forEach(function forEachPassFail (value, key) { });. Notice parameters order is not the same as in jQuery.each method. Appart from IE 8 and below, Array.prototype.forEach is widely supported: http://kangax.github.io/compat-table/es5/#Array.prototype.forEach.

• Avoid multiple DOM call

When we go one step above your code, we can see something like this:

var div = document.getElementById('Store');
div.innerHTML = "";
var content = 'some content';
content += 'whatever'; // multiple times
$(content).appendTo(div);

The variable div is only used twice, so the code could be written as follows:

var content = 'some content';
content += 'whatever'; // multiple times
var div = document.getElementById('Store');
div.innerHTML = "";
$(content).appendTo(div);

And even this could be simplified as:

var content = 'some content';
content += 'whatever'; // multiple times
var div = document.getElementById('Store');
div.innerHTML = content;

This way, you have only one DOM modification, instead of two.

• Don't embed HTML into JavaScript

You should not have code like this var Firstoff = "<div class='someclass'>...</div>. Following preceding advice, you better use document.createDocumentFragment, or even better, a templating system.

• Don't embed CSS into HTML

You should not embed CSS declaration into your HTML code. If you want to add style to an element, use CSS classname for that purpose. Have a look:

From this:

<div class="col-md-1" style="border-color: black;padding-left: 0px;margin-right: 2px;">...</div>

To this:

HTML

<div class="col-md-1 my-style">...</div>

CSS

.my-style {
 border-color: black;
 padding-left: 0px;
 margin-right: 2px;
}

• Be careful with indentation style

Your code looks like as follows:

$(document).ready(
 function timerFired () { // indent += 8, why not ? But why use indentation here ?
 setInterval(function () { // indent += 4, uh ?
 $.ajax({url: "/QcT/bulk"}) // indent += 4, ok...
 .done(function (data) { // indent += 8, uh ??
 // ...
 });
 }, 6000);
 }); // can you guess what does this close ? timerFired (indent=8) & ready call (indent=0)

This one looks like more readable IMHO (but others tips such as "avoid spaghetti code" still apply):

$(document).ready(function onReady () {
 setInterval(function timerFired () {
 $.ajax({url: "/QcT/bulk"})
 .done(function onBulkResultGot (bulkData) {
 // ...
 });
 }, 6000);
});

• JavaScript contains a capital S

But this points doesn't really matters anyway :)

Also, if you're new to JavaScript world, you may have a look to this post: http://tobyho.com/2011/11/16/7-common-js-mistakes-or-confusions/. It explains 7 common mistakes we found in this world. Finally, and as a side note, I can't recommend you enough to check out and use JSHint.

Hope that helps :)

• Josh Kirkpatrick: Thank you very much, So much information here it has helped a lot

Answer 2

Can you explain what you do with the data from the Ajax call ? It seems to me that it is replaced by the data of the JSON since you named both variables the same.

Aside from that :

• You use jQuery, so you can use its selectors, no need to use getElementById
• No need to do var i = 0 and then again in the for loop. I think the declaration in the loop is enough
• You don't seem to use the variables color and time anywhere else than in the line just after the condition to set them, so there is no need to store those informations, you can directly update the content value in the if you currently use to set time and color

• Josh Kirkpatrick: I tried not setting color and time, but it just would not accept it and would give me an error saying it is not defined.
• Loufylouf: Seem strange and I don't see why updating directly your content variable would not work. But it's a minor point, most importantly, why doesn't your script that updates the JSON file directly returns the content of the JSON file, so that you don't have to use getJSON and just have to make the Ajax call to both get the data and have the file updated ?

Answer 3

You are using a bunch of variables (MachineName, Firstoff, comment, content) without declaring them. That means that they will be implicitly created as global variables, or if any of them already exist as global variables they will be used and change any values that they would have.

You are creating HTML code with text concatenated into it. That text should be HTML encoded so that any special characters used in HTML can't break the code. It's often easier to create element directly and use the text method to put the content in them.

$(document).ready(function () {
 setInterval(function () {
 console.log("start");
 $.ajax({url: "/QcT/bulk"})
 .done(function (data) {
 console.log("Done");
 $.getJSON("../../../UpdatedData/QC/QC.json", function (data) {
 var div = $('#Store').text('');
 for (var i = 0; i < data.length; i++) {
 var Machine = data[i];
 //Correctley format the machine name
 var MachineName;
 if (Machine.MachineNo < 10) {
 MachineName = "ZW0100" + Machine.MachineNo;
 } else {
 MachineName = "ZW010" + Machine.MachineNo;
 }
 // Test to see if we have a first off. 
 var Firstoff;
 console.log("Machine" + MachineName + "override " + Machine.FirstoffOverride + " FirstOFfTime " + Machine.FirstoffTime);
 if (Machine.FirstoffTime !== null) {
 Firstoff = $('<div>').css({ borderColor: 'black', backgroundColor: 'greenyellow', color: 'greenyellow' }).text(' 1 ');
 } else {
 Firstoff = $('<div>').css({ visibility: 'hidden', backgroundColor: '#DFDFDF', color: '#DFDFDF' }).text(' 1 ');
 }
 Firstoff.addClass('col-md-1').css({ paddingLeft: 0, marginRight: '2px', borderStyle: 'solid', borderWidth: '2px', '-webkit-border-radius': '27px', '-moz-border-radius': '27px', borderRadius: '27px', paddingRight: 0, fontSize: '30px', textAlign: 'center', width: '46px', marginLeft: '10px' });
 //Test to see if a comment is set
 var comment;
 if (Machine.comment !== null) {
 comment = Machine.comment;
 } else {
 comment = " ";
 }
 var content = $('<div>').addClass('panel panel-default').css({ fontWeight: 700, marginBottom: 0, borderRadius: '46px', backgroundColor: '#DFDFDF', boxShadow: '3px 5px 14px #474747', height: '63px', marginBottom: '11px' }).append([
 $('<div>').addClass('panel-body').css({ padding: '10px' }).append([
 $('<div>').addClass('col-md-8'),
 $('<div>').addClass('col-md-2').css({ fontSize: '33px', color: 'black' }).text(MachineName),
 Firstoff[0],
 $('<div>').addClass('col-md-9 text-center').css({ fontSize: '28px', color: 'Black' }).text(comment)
 ]),
 $('<div>').addClass('col-md-4').append($.map(Machine.PassFail, function(key, value){
 var color, time;
 //Check to see if it's pass or fail
 if (value.password === '0001') {
 color = "greenyellow";
 } else {
 color = "red";
 }
 time = value.ReaderTime;
 return $('<div>').addClass('col-md-1').css({ paddingLeft: 0, marginRight: '2px', borderStyle: 'solid', borderWidth: '2px', '-webkit-border-radius': '27px', '-moz-border-radius': '27px', borderRadius: '27px', paddingRight: 0, backgroundCcolor: color, fontSize: '30px', textAlign: 'center' }).text(time);
 }))
 );
 $(content).appendTo(div);
 }
 console.log("refresh");
 });
 });
 }, 6000);
});