I've never formally learned javascript, and I feel very much like I'm unaware of 'good' practice.

I'm working on this open source project, and in particular the code that converts the incoming JSON into a usable data structure in this file

The relevent function looks like this:

   function start() {
       key = "toppage";
       utterances = {};
       links = {};
       colours = {};
       icons = {};
       labels = {};
       slide_number = {};
       var req = new XMLHttpRequest();
       req.open("GET", "pageset.json");
       req.onreadystatechange = function() {
           if (req.readyState == 4 && req.status == 200) {
               var obj = JSON.parse(req.responseText);
               for (grid in obj.Grid) {
                   labels[obj.Grid[grid][0]] = obj.Grid[grid][1];
                   utterances[obj.Grid[grid][0]] = obj.Grid[grid][2];
                   links[obj.Grid[grid][0]] = obj.Grid[grid][3];
                   icons[obj.Grid[grid][0]] = obj.Grid[grid][4];
                   colours[obj.Grid[grid][0]] = obj.Grid[grid][5];
                   slide_number[obj.Grid[grid][0]] = obj.Grid[grid][6];
               grid_size_rows = obj.Settings[0];
               grid_size_columns = obj.Settings[0];
       //TODO - needs an error message if the JSON doesn't load

I have NO idea if this is a reasonable way to populate structures in javascript. It works, but I feel like it could be a hell of a lot more elegant. Any comments people have would be welcome.

  • \$\begingroup\$ Could you add a document from the response you're receiving? Also, this is not a complete code. Functions setup_*() are missing. CodeReview does not allow posting partial code with links to GitHub. \$\endgroup\$ Commented Dec 27, 2017 at 22:09

2 Answers 2


First of all,you declare them without anything,so it's probably declared globally,this is not good,with this amount of variables in global.

Second of all,ES6 Destructuring assignment

var arr=[1,2,3,4,5];
var [a,b,c,d,e]=arr;
console.log(a,b,c,d,e); // 1,2,3,4,5

Using the same way,

for (grid in obj.Grid) {
    var i=obj.Grid[grid][0];
    var [unused,labels[i], utterances[i], links[i], icons[i], colours[i], slide_number[i]]
  • Declare variables with let (if it gets reassigned) or const (if it never gets reassigned).
    I would recommend against using var nowadays. There are no use cases I know of where it is better than let.

    As far as I can see, all variables in your posted code can be declared const.

  • Do not use for (const entry in obj) pattern for iterating keys. You use it here: for (grid in obj.Grid).

    Modern alternatives:

    for (const [key, value] of Object.entries(obj)) {
      // ...
    for (const key of Object.keys(obj)) {
      const value = obj[key]
      // ...
  • Use === for comparison, e.g. req.status === 200

  • You could use the newer Fetch API as a replacement for XMLHttpRequest. In my opinion, this is worthwhile just for the Promise-driven code you get for free. For example, your //TODO - needs an error message if the JSON doesn't load can be expressed as .catch(err => /* error handling */) (note that fat arrow function syntax).

  • Extract "pageset.json" into a constant: const ASSET_JSON_ENDPOINT = "pageset.json" and put it at the beginning of the function or even outside of the function in the outer scope of your

    • file or
    • module (ES6+ terminology) or
    • your Immediately-Invoked Function Expression (IIFE)

    Generally, I would recommend using modules (with import ... from ... and export ... syntax). They supersede IIFEs.

    Also choose a better name than "asset" depending on the kind of data you're loading.

  • Properly indent/auto-format your code.


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.