Generate URLs from a complex JSON object using recursion

Here's the biggest efficiency issue in fun():

fun(i + 1, 0,dim,raw, fightSpecifics,metfield,URLMetric,URLCollector,Winner); fun(i, j + 1,dim,raw, fightSpecifics,metfield,URLMetric,URLCollector,Winner); 

Let's trace out the calls here.

fun(0,0,...); fun(1,0,...); fun(0,1,...); fun(2,0,...); fun(1,1,...); fun(1,0,...); fun(0,2,...) fun(3,0,...); fun(2,1,...); fun(2,0,...); fun(1,2,...); fun(2,0,...); fun(1,1,...); fun (1,0,...); fun(0,3,...); 

Notice that there are many repeated calls: i=1 j=0 is called on the second, third, and fourth recursion levels. i=2 j=0 is called once on the third and twice on the fourth. It only gets worse from there. You're doing redundant work.

So, you need to either keep track of the ones you've done already in some kind of data structure, or (better in my opinion) get rid of the recursion and the ifs that check on i and j, and create an outer function with some nested for loops that calls into it properly.

Another issue:

 for(var i=0;i<battle[fightDetail].Fighters.length;i++) { fightGeneralDetails= battle[fightDetail].Fighters[i]+ " " + fightGeneralDetails; } 

Any particular reason you're pre-appending here? I'd think that appending to the end would be more efficient. Loop backwards if the order matters.

 if(!(prop=="Random"||prop=="AddToRandom")) { for(var temp in battle[fightDetail].FightDetails[i].AddToRandom) { fightSpecifics = fightSpecifics + (prop+num++ +"="+battle[fightDetail].FightDetails[i][prop]) +"&" ; } } if(prop=="Random") { fun(0,0,battle[fightDetail].FightDetails[i].Random,battle[fightDetail].FightDetails[i].AddToRandom, [],"",fightSpecifics,fightGeneralDetails,battle[fightDetail].FightDetails[i].Winner); } 

Change up the order of the ifs and you can simplify this slightly:

 if(prop=="Random") { fun(0,0,battle[fightDetail].FightDetails[i].Random,battle[fightDetail].FightDetails[i].AddToRandom, [],"",fightSpecifics,fightGeneralDetails,battle[fightDetail].FightDetails[i].Winner); } else if(prop!="AddToRandom")) { for(var temp in battle[fightDetail].FightDetails[i].AddToRandom) { fightSpecifics = fightSpecifics + (prop+num++ +"="+battle[fightDetail].FightDetails[i][prop]) +"&" ; } } 

That this code works is a strong statement in itself. That said, I have no idea how this code works. It befuddles me. So I did what I (try to) do when code befuddles me: I wrapped a test around it so I can see if I break anything right away. Fortunately, this code dumps its output right to the console, so I was able to take that object, copy it into a new file, and write a simple loop that compared all the key-values to make sure nothing changed while I worked on it.

With tests at our backs, we can see that there are some crufty parts to this code.

There is an indentation problem. When I looked at the code here, I had no idea how the code was doing anything. Because of the way it is indented, it looks like the for(fightDetail in battle) loop is outside of the URL() function. After re-indenting the file, turns out it actually is in there.

There are also issues with spacing. Code is more readable when there is proper spacing, such as around operators, after semicolons in for-loop declarations, around assignment, etc. Vertical spacing also helps. Blank lines between for-loops helps differentiate them.

There are unused variables:

var URL = {}; 

and

 var patNum = 1; 

Those can go. There are also "overused" variables in the for-loops in URL(), by which I mean there's a lot of thing[i].otherThing.moreThings[j].value. Having to read all that becomes tiresome quickly, and adds noise to the code. Temporary variables, while not always necessary or useful, can help to clean up some of these.

There is also this line:

u=u+"Winner="+Winner+ "&"+"complete=true"; 

It comes after the assignment to data, and nothing is returned from the function, so that means that this is never used. I suspect that this might be a bug, but to make my tests keep passing, it doesn't show up so I deleted it.

There are a number of places where you do the following:

something = something + "more stuff"; 

Javascript has the += operator, and it works for strings as well as numeric values. So the same behavior can be written as:

something += "more stuff"; 

Variable names provide important context to both the code and the domain it is modelling. The variables in this code don't communicate very effectively. We have names like fun, data, dim, raw, and metfield. These are not descriptive and take some time to tease out. We also have fightSpecifics. It is used in both the for-loops and the fun() function. Great! I can follow that through then. Uh... nope. That value corresponds to the URLMetric parameter, and the fightSpecifics parameter is an empty array.

This code also has a bit of a spaceship going on. This comes from the nesting of conditionals and loops. In every for-in loop, there is a corresponding check on hasOwnProperty. The check is fine, and recommended, but since there are so many loops-in-ifs-in-loops, the indenting grows. But there is a way to avoid this nesting without losing the safety. Rather than doing something like:

for(key in obj) { if(obj.hasOwnProperty(key)) { // do LOTS of stuff } } 

We can instead do:

for(key in obj) { if(!obj.hasOwnPropery(key)) { continue; } // do LOTS of stuff } 

This doesn't make sense if there's only a line or two inside the loop, but when there's a lot of code there, it can help.

Speaking of for-in loops, we don't need to use a for-in loop on an array. There are two places where we are doing that in the code above. This also helps with a related problem where variables are "scoped" much larger than they need to be. I'm looking at you num. (Scoped is in quotes because of the scoping rules of JS.)

We've covered a lot of the surface-level stuff, but now we have to dive into the fun() function. It's recursive. It calls itself twice. It returns nothing. It mucks in a global variable (data). Its base cases are when it goes outside the boundaries of dim. It only does something once we exceed the row length (and does nothing on exceeding the column length). It takes values from two-dimensional dim and shoves them into one-dimensional fightSpecifics. I have no idea what is going on here.

Time to look at the output. Turns out, this function is supposed to permute the values in the battle.Combat.FightDetails[0].Random array, and build a URL with them. I would not have guessed that from looking at the code.

There's a lot going on here. First, there are two responsibilities in this code. We can pull out the "build a URL" part into its own function. This is literally exactly what is sounds like:

function fun(i, j, dim, raw, fightSpecifics, metfield, URLMetric, URLCollector, Winner) { if (i >= dim.length) { buildURL(i, raw, fightSpecifics, metfield, URLMetric, URLCollector, Winner); return; } ... } function buildURL(i, raw, fightSpecifics, metfield, URLMetric, URLCollector, Winner) { var num = 1; var u = "www.fightmosnters.com/?" + URLMetric; for (var k = 0; k < i; k++) { metfield += " " + fightSpecifics[k]; } for(prop in raw) { if(raw.hasOwnProperty(prop)) { var fin = ""; fin = "fightSpecifics=(" + metfield + " " + raw[prop] + ")"; u += "FightDetails" + num++ + "=" + URLCollector + " " + fin + "&"; } } data[metfield] = u; } 

Now we can isolate changes to each without worrying about breaking the other inadvertently. We can return instead of mucking in a global, and we can pull out building metfield so we can have that available at a higher level. Also, rather than looping and concatenating all the strings in an array with a separator, we can use Array.prototype.join(separator), so:

metfield = " " + fightSpecifics.join(" "); 

Speaking of metfield, turns out it is set as an empty string, passed around as an empty string, and only used in the new buildURL() function. We can eliminate it as a parameter, and create a local variable instead.

We now have the recursive part separated, and we can deal with it on its own. The recursive part, we discovered earlier, is permuting an array. We can probably develop a better permutation algorithm that doesn't cover the same ground multiple times. Here's what that might look like:

function permute(values, row, soFar, aggregator) { aggregator = aggregator || []; if(row >= values.length) { aggregator.push(soFar); return; } for(var i = 0; i < values[row].length; i++) { permute(values, row + 1, soFar + " " + values[row][i], aggregator); } return aggregator; } 

We can then rewrite our fun() function to use it:

function fun(dim, raw, URLMetric, URLCollector, data) { data = data || {}; var fights = permute(dim, 0, ""); for(var k = 0; k < fights.length; k++) { var metfield = fights[k]; data[metfield] = buildURL(raw, metfield, URLMetric, URLCollector); } return urls; } 

With that, we've now eliminated a bunch of parameters and made the code much cleaner and more succinct. It's also clear that we are permuting an array, and using that to build a URL.

After working with the code, the final result looks like:

function fun(dim, raw, urlMetric, urlCollector, urls) { urls = urls || {}; var fights = permute(dim, 0, ""); for(var k = 0; k < fights.length; k++) { var metfield = fights[k]; urls[metfield] = buildURL(raw, metfield, urlMetric, urlCollector); } return urls; } function permute(values, row, soFar, aggregator) { aggregator = aggregator || []; if(row >= values.length) { aggregator.push(soFar); return; } for(var i = 0; i < values[row].length; i++) { permute(values, row + 1, soFar + " " + values[row][i], aggregator); } return aggregator; } function buildURL(raw, metfield, urlMetric, urlCollector) { var u = "www.fightmosnters.com/?" + urlMetric; for(var k = 0; k < raw.length; k++) { var fin = "fightSpecifics=(" + metfield + " " + raw[k] + ")"; u += "FightDetails" + (k + 1) + "=" + urlCollector + " " + fin + "&"; } return u; } module.exports = function URL() { var battle = { "Combat": { "Fighters": [ "Fighter1", "Fighter2" ], "FightDetails": [ { "FightName": "Plassey", "Era": "Medieval", "BeginDate": "14th Century", "End Date": "17th Century", "Random": [ [ "Random_1_1", "Random_1_2" ], [ "Random_2_1", "Random_2_2" ], [ "Random_3_1", "Random_3_2", "Random_3_3" ] ], "AddToRandom": [ "AddedRandomness1", "AddedRandomness2", "AddedRandomness3" ], "Winner":"Goku" } ] } }; // BUG?: This assumes only one fightDetail in battle for(var fightDetail in battle) { if(!battle.hasOwnProperty(fightDetail)) { continue; } var battleDetail = battle[fightDetail]; var fightGeneralDetails = battleDetail.Fighters.reverse().join(" ") + " "; var urls = {}; for(var i = 0; i < battleDetail.FightDetails.length; i++) { var fightDetails = battleDetail.FightDetails[i]; var fightSpecifics = ""; // BUG: No longer breaks on order, but we have to filter out Winner for(var prop in fightDetails) { if(!fightDetails.hasOwnProperty(prop)) { continue; } if(["Random", "AddToRandom", "Winner"].indexOf(prop) !== -1) { continue; } for(var num = 1; num <= fightDetails.AddToRandom.length; num++) { fightSpecifics += (prop + num + "=" + fightDetails[prop]) + "&"; } } // BUG: If there is more than one battleDetail.FightDetails, this clobbers previous data // Passing in data as a parameter *might* alleviate this, but there can still be some clobbering urls = fun(fightDetails.Random, fightDetails.AddToRandom, fightSpecifics, fightGeneralDetails, urls); } return urls; } } 

I did this in many small steps. To follow the steps as they happened (which isn't exactly the same order as described), see https://bitbucket.org/cbojar-codereview/generate-urls-from-a-complex-json-object-using-recursion

There is more refactoring that could be done, but this seems like a good place to stop for now.

There are a few potential bugs noted in the code that should be looked at. Also, I am wondering if www.fightmosnters.com should be www.fightmonsters.com.