Can I use a loop to optimize my code?

Question

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!

if you have two arrays you will have to iterate through them. It seems like you your code you will need a nested for loop for vehicles and in it for aircraft. This is O(n^2) which is not great. Indexing a specific location in a hash table (which I am assuming you have have hash table will take O(1) per look up, which in total can take O(n) if you have n elements and go through all of them can you explain what your code is trying to do? — Quantico
– Quantico, Commented Nov 17, 2014 at 19:58
Why do you need 2 specific arrays here ? Why not club into a single object ? — Sushanth --
– Sushanth --, Commented Nov 17, 2014 at 20:03
You should alias repeated object access if you care about perf. var foo = game.currentLevel.requirements.vehicles; and then foo.indexOf('blah') — Jared Smith
– Jared Smith, Commented Nov 17, 2014 at 20:03
In a game I have an object named startport. When the starport is selected by the player, it will be possible to select either vehicles or aircrafts in a sidebar menu, as long as the player has sufficient funds. The code is working as it is now, but I believe it could be better with some kind of loop, I'm just not sure how to write it? — Tommy Otzen
– Tommy Otzen, Commented Nov 17, 2014 at 20:12
@Sushanth-- game.currentLevel.requirements.aircraft.indexOf and game.currentLevel.requirements.vehicles.indexOf are to different lines. — Tommy Otzen
– Tommy Otzen, Commented Nov 17, 2014 at 20:17

six fingered man · Accepted Answer · 2014-11-17 20:57:46Z

3

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;
        }
    }
}

edited Nov 17, 2014 at 20:57

answered Nov 17, 2014 at 20:03

six fingered man

2,50016 silver badges16 bronze badges

Sign up to request clarification or add additional context in comments.

14 Comments

charles Over a year ago

Since there's a finite number of possibilities here, typically the most optimized way to handle this would be to completely unroll the loop.

six fingered man Over a year ago

@charles: The question already has the unrolled version. He's asking how to shorten code with loops. Not all code needs to be optimized. Sometimes clarity and scalability is more important.

six fingered man Over a year ago

@charles: Also keep in mind that this code relies on operations that are already very expensive (DOM selection/manipulation). The test you posted is with an extremely light weight operation. So in this case any performance gain would never be noticed.

charles Over a year ago

to quote, "I'm trying to optimize my code to be more efficient and easier to read." He's wanting his cake and to eat it too. You're achieving the second half of the statement, but moving in the wrong direction for the first.

six fingered man Over a year ago

I updated with a version that drops the jQuery and the .forEach().

|

Collectives™ on Stack Overflow

Can I use a loop to optimize my code?

1 Answer 1

14 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

14 Comments

Your Answer

Sign up or log in

Post as a guest

Related