Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

There has to be a better way of checking on variables than this nightmare of ifs. It works, but I don't think it's good code at ALL.

My HTML has three divs, each id tagged with the logic behind my object (ma/shadow/traps) which contain 10 div each (every icons), also id tagged according to the object. On click, I'm gathering the id and the parent id to traverse the object and find the value.

It's only possible to raise the value if the pre-requirement is not 0, or if the pre-requirement is false (no pre-req). My problem comes when there are 2 pre-requirements which need to be checked. I check the first one, and if the first meets the pre-req, I check if a second one exists, and check it.

I'm not looking for a full copy/paste code here, but more for a more efficient logic on how to do what I'm already doing.

See HTML : http://veksen.net/d2/dev-just-skills.html and JS : http://veksen.net/d2/dev-script-skills2.js

$(function () {
var basemin = 0;
var basemax = 20;
var skill = {
    ma:{
    //sample object
    },
    shadow:{
        clawMastery:{
            base:0,
            preReq:false
        }, psychicHammer:{
            base:0,
            preReq:false
        }, burstOfSpeed:{
            base:0,
            preReq:'clawMastery'
        }, weaponBlock:{
            base:0,
            preReq:'clawMastery'
        }, cloakOfShadows:{
            base:0,
            preReq:'psychicHammer'
        }, fade:{
            base:0,
            preReq:'burstOfSpeed'
        }, shadowWarrior:{
            base:0,
            preReq:'cloakOfShadows',
            preReq2:'weaponBlock'
        }, mindBlast:{
            base:0,
            preReq:'cloakOfShadows'
        }, venom:{
            base:0,
            preReq:'fade'
        }, shadowMaster:{
            base:0,
            preReq:'shadowWarrior'
        }
    },
    traps:{
    //sample object
    }
};
$('#tree div').bind("contextmenu", function (e) {
    e.preventDefault();
});
$('#tree .tab div').mousedown(function (e) {
    var $this = $(this);
    $this.attr('unselectable', 'on').css('UserSelect', 'none').css('MozUserSelect', 'none');
    var $tab = $this.parent().attr("id");
    var $skill = $this.attr("id");
    function checkPreReq(e) {
        var $preReq = skill[$tab][e]['preReq'];
        var $preReq2 = skill[$tab][e]['preReq2'];
        alert($preReq);
        if ($preReq === false) {
            alert('no pre-req');
            return true;
        } else if ($preReq !== false) {
            alert('pre-req1 exists');
            if(skill[$tab][$preReq]['base'] > 0) {
                alert('pre-req1 higher than 0');
                if ($preReq2 === undefined) {
                    alert('no second pre-req, go on');
                    return true;
                } else {
                    alert('check second pre-req value');
                    if (skill[$tab][$preReq2]['base'] > 0) {
                        alert('pre-req2 higher than 0');
                        return true;
                    } else {
                        alert('pre-req2 not met (0)');
                        return false;
                    }
                }
            } else {
                alert('pre-req1 not met (0)');
                return false;
            }
        } else {
            alert('else');
            return false
        }
    }
    if (e.which == 1) {
        //leftclick
        if (skill[$tab][$skill]['base'] < basemax && checkPreReq($skill)) {
            skill[$tab][$skill]['base'] += 1;
            //$rem_skills -= 1;
        }
    } else if (e.which == 3) {
        //rightclick
        if (skill[$tab][$skill]['base'] > basemin) {
            skill[$tab][$skill]['base'] -= 1;
            //$rem_skills += 1;
        }
    }
    $this.find(".lvl").text(skill[$tab][$skill]['base']);
    //$("#output").find(".rem_skills").text($rem_skills);
});
});
share|improve this question

migrated from stackoverflow.com Feb 4 '13 at 2:43

This question came from our site for professional and enthusiast programmers.

3 Answers 3

up vote 4 down vote accepted

If there are zero, one, two or possible more prerequirements, generalize their storage. Do not use multiple properties and nested if-else-statements, but an array and a loop.

The function should then look like this:

function checkPreReq(e) {
    var preReqs = skill[tab][e].preReqs;
    if (!preReqs)
        return true;
    for (var i=0; i<preReqs.length; i++) {
        var preReq = preReqs[i];
        if (skill[tab][preReq].base <= 0)
            // failed at least this preReq
            return false;
    }
    // passed all requirements
    return true;
}
share|improve this answer
1  
+1 Yes, same comment as for RoneRackal's answer. Also, looping with $.each() would simplify the code slightly, but that's optional. –  Beetroot-Beetroot Feb 3 '13 at 23:15
    
I cannot seem to extract the array, preReqs thus always returns true (being false/undefined), my array now looks like shadowWarrior:{ base:0, preReqs:{ 1:'cloakOfShadows', 2:'weaponBlock' } } –  user1732521 Feb 3 '13 at 23:41
    
@user1732521: That's not an array. Use { base:0, preReqs: ['cloakOfShadows', 'weaponBlock'] } –  Bergi Feb 3 '13 at 23:47
    
@Beetroot-Beetroot: I can't see why $.each would simplify anything. If at all, you might use .every –  Bergi Feb 3 '13 at 23:48
    
thanks... for some reason array and object were the same in my head –  user1732521 Feb 3 '13 at 23:51

What I would probably do is each skill has a 'prereqs' array, which contains links to the prereq skill.

Then when you click the skill, you just loop through the prereqs and check that each one has skillpoint > 1

This would also satisfy the first skills that have no prereqs, they will just skip the loop because their prereq array would be empty.

This also allows if you ever need more than just 2 prereqs, since the array can have as many prereqs as you wan't and you won't have to add more 'if' checks

share|improve this answer
1  
+1 Yes exactly. Nobody in their right mind would do this any other way. –  Beetroot-Beetroot Feb 3 '13 at 23:06

I'm not quite sure I've followed your logic, but it sounds like you have two pre-requisites (either of which might not be present), and you want to return false in any case where a prerequisite is present but is not met.

Can't you break out the testing of one prerequisite into its own function to simplify the logic? Something like this:

function checkOnePreReq(isRequired, value) {
    return !isRequired || value > 0;
}

function checkPreReq(e) {
    var $preReq = skill[$tab][e]['preReq'];
    var $value = skill[$tab][$preReq]['base'];

    var $preReq2 = skill[$tab][e]['preReq2'];
    var $value2 = skill[$tab][$preReq2]['base'];

    return checkOnePreReq($preReq, $value) &&
           checkOnePreReq($preReq2, $value2);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.