Generate URLs from a complex JSON object using recursion

Question

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 < i; k++)
        {
            metfield=metfield + " " + fightSpecifics[k];
        }
        for(prop in raw)
        {
            if(raw.hasOwnProperty(prop))
            {
                var fin ="";
                fin = "fightSpecifics=(" + metfield +" "+ raw[prop]+")";
                u=u+"FightDetails"+ num++ +"=" +URLCollector+" "+fin+"&";
            }
        }
        data[metfield]=u;
        u=u+"Winner="+Winner+ "&"+"complete=true";
        return;
    }
    if(j >= 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":[
            {
                "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"
            }
        ]
    }
};
        for(var fightDetail in battle)
        {
            var fightGeneralDetails="";
            console.log(fightDetail);
            if(battle.hasOwnProperty(fightDetail))
            {
                for(var i=0;i<battle[fightDetail].Fighters.length;i++)
                {
                    fightGeneralDetails= battle[fightDetail].Fighters[i]+ " " + fightGeneralDetails;
                }
                var num=1;
                for(var i=0;i<battle[fightDetail].FightDetails.length;i++)
                {
                    var fightSpecifics="";
                    for(var prop in battle[fightDetail].FightDetails[i])
                    {
                        num=1;
                        if(battle[fightDetail].FightDetails[i].hasOwnProperty(prop))
                        {
                            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);
                            }
                        }
                    }
                }
                console.log(data);
                return data;
            }
        }

}

EDIT : Adding Express JS code for reference (Can be ignored)

var express = require('express');
var app = express();
app.get("/",function(req,res){
  var t = require(<Add Path to The JS File>);
  t();
})
app.listen(3001);
module.exports = app;

Answer 1

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.

Answer 2

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]) +"&" ;
             }
         }