JSON to data structure in Javascript

Question

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.overrideMimeType("application/json");
 req.send(null);
 req.onreadystatechange = function() {
 if (req.readyState == 4 && req.status == 200) {
 var obj = JSON.parse(req.responseText);
 for (grid in obj.Grid) {
 console.log(obj.Grid[grid][0])
 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];
 if(obj.Grid[grid][6]==0){
 key=obj.Grid[grid][0]
 }
 }
 grid_size_rows = obj.Settings[0];
 grid_size_columns = obj.Settings[0];
 setup_messagewindow();
 setup_table();
 load_page(key);
 }
 };
 //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.

• Igor Soloydenko: 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.

Answer 1

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]]
 =obj.Grid[grid]
 //....
}

Answer 2

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