Generate URLs from a complex JSON object using recursion

My task is here as follows :


Input : A complex/nested JSON object with nested arrays as well
objects


Output : A JSON object that contains a set of URLs with their titles:
( key,value ) pair eg {"Title1":"URL1","Title2":"URL2"}

In this code, I simply go through the JSON object battle. My task is to append all these values and form a URL string. The only catch is that the number of URLs is a cross product of the number of elements in the Random property of the FightDetails property. FightDetails is defined as an array as there can be more than 1 Fight. The number of URLs generated for each fight can be calculated by : Combat.FightDetails[i].Random[0].length X Combat.FightDetails[i].Random[1].length X Combat.FightDetails[i].Random[2].length X..... X Combat.FightDetails[i].AddToRandom.numberOfElements.

fun() is a recursive function

PS: The variable naming may be awkward (hence ignore), but that is due to privacy concerns.

I have the following Javascript Code to generate the set of URLs.

It runs pretty well. I just want to know if there are any redundancies or quality improvements.

NOTE: I'm running this on an expressjs server. Please let me know in case someone needs the express app code to be able to run that. I have made the URL() function exportable.

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

module.exports = function URL()
{
var patNum=1;
var battle = {
"Combat":{
"Fighters":[
"Fighter1",
"Fighter2"
],
"FightDetails":[

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

StackExchange Code Review Q#90450, answer score: 2