Acquiring and formatting data from a JSON file

Question

I have a JavaScript function that is getting data out of a JSON file and then doing some formatting to it. The code works but I feel like it just an awful way of doing it. All criticism and help appreciated.

function MCScrap(ZWA, ZWB, ZWC, ZWD, Number) {

$(document).ready(
        function() {
            setInterval(function() {



                $.getJSON("mccount.json", function(machines) {
                    console.log(machines);
                    var Tcount = machines.Machines[Number].Tcount;
                    var Ycount = machines.Machines[Number].Ycount;
                    var TScount = machines.Machines[Number].TScount;
                    var YScount = machines.Machines[Number].YScount;
                    document.getElementById(ZWA).innerHTML = Ycount;
                    document.getElementById(ZWB).innerHTML = Tcount;
                    document.getElementById(ZWD).innerHTML = TScount + "%";
                    document.getElementById(ZWC).innerHTML = YScount + "%";
                    if (TScount < 3) {
                        document.getElementById(ZWB).style.backgroundColor = 'lime';
                    } else if (TScount < 5) {
                        document.getElementById(ZWB).style.backgroundColor = 'orange';
                    } else {
                        document.getElementById(ZWB).style.backgroundColor = 'red';
                    }

                    if (YScount < 3) {
                        document.getElementById(ZWA).style.backgroundColor = 'lime';
                    } else if (YScount < 5) {
                        document.getElementById(ZWA).style.backgroundColor = 'orange';
                    } else {
                        document.getElementById(ZWA).style.backgroundColor = 'red';
                    }
                }
                );

            }, 3000);
        });
}

jfriend00 · Accepted Answer · 2014-07-21 10:51:14Z

I'd suggestion cleaning it up like this:

function MCScrap(ZWA, ZWB, ZWC, ZWD, Number) {
    $(document).ready(function () {
        setInterval(function () {
            $.getJSON("mccount.json", function (machines) {
                var zwa, zwb, zwc, zwd, TScount, YScount;

                zwa = document.getElementById(ZWA);
                zwb = document.getElementById(ZWB);
                zwc = document.getElementById(ZWC);
                zwd = document.getElementById(ZWD);

                zwa.innerHTML = machines.Machines[Number].Ycount;
                zwb.innerHTML = machines.Machines[Number].Tcount;

                YScount = machines.Machines[Number].YScount;
                zwc.innerHTML = YScount + "%";
                TScount = machines.Machines[Number].TScount;
                zwd.innerHTML = TScount + "%";

                function getColor(count) {
                    if (count < 3) {
                        color = 'lime';
                    } else if (count < 5) {
                        color = 'orange';
                    } else {
                        color = 'red';
                    }
                    return color;                
                }

                zwa.style.backgroundColor = getColor(YScount);
                zwb.style.backgroundColor = getColor(TScount);

            });

        }, 3000);
    });
}

Types of changes:

Get a DOM element once into a variable and then use it from there rather than calling document.getElementById() more than one for the same DOM element.
Put the color code into a reusable function rather than having two copies which also makes it easier to set the color in just one place rather than three places.
Put zwa, zwb, zwc, zwd blocks of code in order to make it easier to read and follow.

Some other things that seem a little off to me:

Putting $(document).ready() inside another function like this seems a little odd. Usually, you would call MCScrap() inside a $(document).ready() as that would make your function a bit more reusable.
Setting up an ajax call to run every 3 seconds in perpetuity is generally not a good idea. For starters, it's horrible for battery life (phone, tablet, laptop). And, it can get messy if you every get in a situation where the ajax call takes longer than 3 seconds to finish or timeout. Then, you have multiple ajax calls in flight at the same time and they could be building up. The safer way to do this is to launch the next ajax call from a setTimeout() that is set in the completion of the previous ajax call. Then, they can never build up.

Thank you very much the concernes about the three seconds are not an issue as it's a dedicated web app for a business that needs to be up to date however i did change it to SetTimeout i have also used the function for changing the colours it's much nice thank you — Josh Kirkpatrick
– Josh Kirkpatrick, Commented Jul 21, 2014 at 11:32

Flambino · Accepted Answer · 2014-07-21 18:35:38Z

Indentation is all over the place
Variable names are terrible. Sorry, but they really, really are. I'm not sure how you keep track of them - I certainly can't. Be more descriptive. I have zero clue what ZWB is supposed to be, or even what it's supposed to be relative to, say, ZWA. And there are no comments to explain any of it either.
Variable names in JavaScript should also be camelCase. You have an argument called Number (capital "N") and I'm a little surprised that actually works, since Number is a core JavaScript prototype (i.e. class). Prototype contructors (classes) should be PascalCase, while functions and variables should be camelCase. (And I wouldn't be surprised if your code fails in some browsers, because you're basically trying to redefine Number)
Extract the background color code into its own function, and avoid the duplication.
The whole structure seems weird. Named function containing an anonymous callback function (the "ready" handler) containing an anonymous callback function (getJSON) containing an anonymous callback function (setInterval)... wow.
Make the innermost code (getJSON) its own function, and set up the interval (or better yet, setTimeout) and everything else somewhere else.
Why aren't you using jQuery everywhere? You're obviously using it for getJSON etc., but you're not using any of the DOM manipulation functions, which is the bread and butter of jQuery.

// I've left the names alone; don't know what to call them
// But I'm going to assume you refactor your code to pass
// in either jQuery selections or selectors
function updateMachines(zwa, zwb, zwc, zwd, machineIndex) {

  function getBackgroundColor(count) {
    if(count < 3) return 'lime';
    if(count < 5) return 'orange';
    return 'red';
  }

  $.getJSON("mccount.json", function(machines) {
    var machine = machines.Machines[machineIndex];

    $(zwa)
      .text(machine.yCount)
      .css('background-color', getColor(machine.ysCount));

    $(zwb)
      .text(machine.tCount)
      .css('background-color', getColor(machine.tsCount));

    $(zwc).text(machine.ysCount + "%");
    $(zwd).text(machine.tsCount + "%");
  });
}


// set-up polling etc. somewhere else
$(document).ready(function () {
  var pollingTimer = setInterval(updateMachines, 3000);

  // ....
});

Stack Exchange Network

Acquiring and formatting data from a JSON file

2 Answers 2

You must log in to answer this question.

Hot Network Questions

Acquiring and formatting data from a JSON file

2 Answers 2

You must log in to answer this question.

Related

Hot Network Questions