Return to Answer

link to answer instead of question, add missing `y` character on function name

edited Dec 11, 2018 at 17:55

Sᴀᴍ Onᴇᴌᴀ ♦

• 29.5k
• 16
• 45
• 202

Inline Side-by-side Side-by-side Markdown

replaced http://stackoverflow.com/ with https://stackoverflow.com/

edited May 23, 2017 at 12:40

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

answered Feb 13, 2015 at 13:41

pomeh

• 354
• 1
• 5

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: http://stackoverflow.com/questions/10649867/should-a-javascript-function-be-written-and-called-inside-jquery-document-ready. 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.read 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 :)