Sorting variables for human weight/length calculations

Question

I'm trying to simplify this function, as there can be multiple type of data objects and for each type there is also a male and a female version. The number and name of the elements in the objects are always identical.

As you see, most of the code is repeating...

function calculate(type, j, value, s) {
    for (var i = j; i > 4; i--) { 
        if (type == 'weight') {
            if (s == 'f') { 
                if (weightFemale.hasOwnProperty(i)) {
                    var m = weightFemale[i][0],
                        l = weightFemale[i][1],
                        s = weightFemale[i][2];     
                    return getcalc( m,l,s );
                }
            }
            else {
                if (weightMale.hasOwnProperty(i)) {
                    var m = weightMale[i][0],
                        l = weightMale[i][1],
                        s = weightMale[i][2];       
                    return getcalc( m,l,s );
                }               
            }
        }
        else if (type == 'length') {
            if (s == 'f') { 
                if (lengthFemale.hasOwnProperty(i)) {
                    var m = lengthFemale[i][0],
                        l = lengthFemale[i][1],
                        s = lengthFemale[i][2],
                    return getcalc( m,l,s );
                }
            }
            else {
                if (lengthMale.hasOwnProperty(i)) {
                    var m = lengthMale[i][0],
                        l = lengthMale[i][1],
                        s = lengthMale[i][2],
                    return getcalc( m,l,s );
                }               
            }
        }
    }
    return false;
}

How can I simplify the if/else-parts for the type and the sex?

Answer 1

if (s = 'f') { 

It seems that you are trying to do some comparison here (considering this is in a for-loop). However, you are using the assignment operator so this will always evaluate to true)

---

            if (s == 'f') { 
                if (weightFemale.hasOwnProperty(i)) {
                    var m = weightFemale[i][0],
                        l = weightFemale[i][1],
                        s = weightFemale[i][2];     
                    return getcalc( m,l,s );
                }
            }
            else {
                if (weightMale.hasOwnProperty(i)) {
                    var m = weightMale[i][0],
                        l = weightMale[i][1],
                        s = weightMale[i][2];       
                    return getcalc( m,l,s );
                }               
            }

These two inner if-statements are virtually identical, all except for weightMale and weightFemale. Duplicating code is bad practice, so I recommend that you use a ternary operator to decide whether or not you are going to use weightMale or weightFemale. Then, just execute using this new information.

Here is the ternary operator:

var weight = s == 'f' ? weightFemale : weightMale;

Now that you have this new weight variable, you can just use that when running the rest of the code, rather than the specific weightMale or weightFemale:

           if (weight.hasOwnProperty(i)) {
                var m = weight[i][0],
                    l = weight[i][1],
                    s = weight[i][2];       
                return getcalc( m,l,s );
            }

The same exact concept can be applied to the height section.

---

This is a rather strange way to see if an index exists in an array:

if (weightFemale.hasOwnProperty(i)) {

I think it would be better to check against the length of the array like this:

if (i < weightFemale.length) {

This looks better because then someone is just quickly looking over your code won't get tripped up and think that weightFemale is actually an object (yeah, everything is an object in JavaScript; I know. You get the point right?)

---

Here is what the code looks like now:

function calculate(type, j, value, s) {
    for (var i = j; i > 4; i--) { 
        if (type == 'weight') {
            var weight = s == 'f' ? weightFemale : weightMale;
            if (i < weight.length) {
                    var m = weight[i][0],
                        l = weight[i][1],
                        s = weight[i][2];       
                    return getcalc( m,l,s );
            }   
        }
        else if (type == 'length') {
                var length = s == 'f' ? lengthFemale : lengthMale;
                if (i < length.length) {
                        var m = length[i][0],
                            l = length[i][1],
                            s = length[i][2];       
                        return getcalc( m,l,s );
                }   
            }
        }
    }
    return false;
}

Answer 2

Basic Improvements

In addition to Sir Python's answer, I've listed some general improvements that could be applied to your code. They're more geared towards design but you may find them helpful, specifically in the case if you were to ever have to come back and change something in it (say 6 months from now?).

• Naming - Your function is called calculate. What is it calculating? Add to the name to explain its purpose further. Secondly, it's returning getcalc(l,m,s) or false. This does not appear to be the result of a calculation (I could be wrong but given that you haven't yet told us anything about your code I can't really make any assumptions).
• Naming v2 - What are j, s, l, & m? Names like these imply indexes/iterators which doesn't appear to be what they are. Expand on their names even if it's just maxCount, size, length, mass or whatever they may be.
• Magic numbers - What is the significance of i > 4? Why 4? Making this a constant/variable will improve readability/maintainability of your code.
• Random Variables - Where are weightFemale/Male and lengthFemale/Male variables coming from? Globals? Why aren't they parameters? Consider making them so.

Answer 3

First, you've got a few errors in the following:

    else if (type == 'length') {
        if (s == 'f') { 
            if (lengthFemale.hasOwnProperty(i)) {
                var m = lengthFemale[i][0],
                    l = lengthFemale[i][1],
                    s = lengthFemale[i][2], // <--- here
                return getcalc( m,l,s );
            }
        }
        else {
            if (lengthMale.hasOwnProperty(i)) {
                var m = lengthMale[i][0],
                    l = lengthMale[i][1],
                    s = lengthMale[i][2],  // <--- here
                return getcalc( m,l,s );

When you're redacting the var keyword by comma, you need to finish with a semicolon.

Also, in return getcalc( m,l,s ) your spacing is wrong. You don't need extraneous space in your brackets, and you should have a single whitespace after your commas.

Also, value is unused, so it can be removed.

---

Data Structures:

At current, your data structure looks like the following:

• weightFemale, weightMale, lengthFemale, lengthMale: two dimensional arrays

Instead of using arrays, use objects instead:

With a data structure like the following:

var data = {
    female: {
        weight: [
            {m: 4, l: 5, s: 7},
            {m: 4, l: 5, s: 7}
        ],
        length: [
            {m: 4, l: 5, s: 7}
        ]
    },
    male: {
        weight: [
            {m: 4, l: 5, s: 7},
            {m: 4, l: 5, s: 7}
        ],
        length: [
            {m: 4, l: 5, s: 7}
        ]
    },
}

Then, assuming you pass s in as female or male instead of f/m:

The following should work:

function calculate(j, sex, type) {
    for (var i = j; i > 4; i--) {
        if (data[sex][type].hasOwnProperty(i)){
            return getcalc(
                data[sex][type][i].m,
                data[sex][type][i].l,
                data[sex][type][i].s
            );
        }
    }
    return false;
}

---

Clarity:

Your program is unclear, the variables are named with single letters, which is bad practice as it is unclear and potentially hard to maintain in the future.

Consider using entire words instead of letters.