Tommy Otzen
Tommy Otzen

Reputation: 190

Can I use a loop to optimize my code?

I'm trying to optimize my code to be more efficient and easier to read. I have some combined if-statements, which I think could be better, if they are transformed to for-loops, I'm just not sure how to do this?

This is my code:

    if (starportSelected){
        if(game.currentLevel.requirements.vehicles.indexOf('transport')>-1 && cashBalance>=vehicles.list["transport"].cost){
            $("#transportbutton").removeAttr("disabled");
        }
        if(game.currentLevel.requirements.vehicles.indexOf('scout-tank')>-1 && cashBalance>=vehicles.list["scout-tank"].cost){
            $("#scouttankbutton").removeAttr("disabled");
        }
        if(game.currentLevel.requirements.vehicles.indexOf('heavy-tank')>-1 &&cashBalance>=vehicles.list["heavy-tank"].cost){
            $("#heavytankbutton").removeAttr("disabled");
        }
        if(game.currentLevel.requirements.vehicles.indexOf('harvester')>-1 && cashBalance>=vehicles.list["harvester"].cost){
            $("#harvesterbutton").removeAttr("disabled");
        }
        if(game.currentLevel.requirements.aircraft.indexOf('chopper')>-1 && cashBalance>=aircraft.list["chopper"].cost){
            $("#chopperbutton").removeAttr("disabled");
        }
        if(game.currentLevel.requirements.aircraft.indexOf('wraith')>-1 && cashBalance>=aircraft.list["wraith"].cost){
            $("#wraithbutton").removeAttr("disabled");
        }   
    }

I think first step would be to create two arrays, one for vehicles and one for aircrafts like this:

    var vehicles = ['transport', 'scout.tank', 'heavy-tank', 'harvester'];
    var aircraft = ['chopper', 'wraith'];

But how to take the rest of the code and change it to for-loop seems like a hard case for me. All help and explanation would be highly appreciated!

Upvotes: 2

Views: 107

Answers (1)

six fingered man
six fingered man

Reputation: 2500

Seems that you have "vehicles" and "aircraft" types, with multiple values for each.

As such, I'd create an object of types to arrays of values.

Because you're also using a variable named vehicles and aircraft, you'll want to reference those in a separate object so that you can look them up with a string.

var lists = {
    vehicles: vehicles,
    aircraft: aircraft
}

var obj = {
    vehicles: ["transport", "scout-tank", "heavy-tank", "harvester"],
    aircraft: ["chopper", "wraith"]
};

Then use an outer and inner loop.

//        v---("vehicles" or "aircraft")
for (var type in obj) { //      v---("transport", "scout-tank", "chopper", etc...)
    obj[type].forEach(function(val) {
        if(game.currentLevel.requirements[type].indexOf(val)>-1 && 
                     cashBalance >= lists[type].list[val].cost) {
            $("#" + val.replace("-", "") + "button").removeAttr("disabled");
        }
    });
}

Notice also that I had to replace the hyphen in the ID selector, since it wasn't used as part of the ID.


The two objects at the top could be combined into a single one if you wish:

var obj = {
    vehicles: {
        list: vehicles,
        values: ["transport", "scout-tank", "heavy-tank", "harvester"]
    },
    aircraft: {
        list: aircraft,
        values: ["chopper", "wraith"]
    }
};

Then adjust the loop references accordingly.

I've also cached the objects for performance, as Jared noted above.

for (var type in obj) {
    var list = obj[type].list;
    var requirements = game.currentLevel.requirements[type];

    obj[type].values.forEach(function(val) {
        if(requirements.indexOf(val)>-1 && cashBalance >= list[val].cost) {
            $("#" + val.replace("-", "") + "button").removeAttr("disabled");
        }
    });
}

To make it even more efficient than the original, we'll drop some of the jQuery calls.

for (var type in obj) {
    var list = obj[type].list;
    var requirements = game.currentLevel.requirements[type];

    for (var i = 0, vals = obj[type].values; i < vals.length; i++) {
        var val = vals[i];
        if(requirements.indexOf(val) > -1 && cashBalance >= list[val].cost) {
            document.getElementById(val.replace("-", "") + "button").disabled = false;
        }
    }
}

Upvotes: 3

Related Questions