1
\$\begingroup\$

I'm working on a function to sort two elements using several properties. I want to know if the function is well structured.

function comparePropsOfAWithB(a, b, props) {
    var prop  = props[0];
    var propA = a[prop];
    var propB = b[prop];

    if (propA === propB) {
        var array = props;
        array.shift();
        return props.length ? comparePropsOfAWithB(a, b, props) : false
    } else if (propA < propB ) {
        return true
    } else {
        return false
    }
}

This function is meant to be used alongside the sort method like this:

array.sort(function(a,b) { return comparePropsOfAWithB(a,b,["a","b","c"]) })

The intent of the function is to compare two objects using several values to determine if they are different. In the example, the key "a" will be tested, and if it fails, "b" will be tested until the last element of the array is reached.

\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

That's not how the comparison function for Array.sort() works at all: it should return negative, zero, or positive.

Eating up all or part of the props argument (due to your array.shift() call) is bad practice.

The usage is cumbersome:

array.sort(function(a,b) { return comparePropsOfAWithB(a,b,["a","b","c"]) })

Wouldn't it be nicer to write a propertyComparator(…) function that returns a function?

array.sort(propertyComparator('a', 'b', 'c'))

Something like this:

function propertyComparator() {
    var props = arguments;
    return function(a, b) {
        for (var i = 0; i < props.length; i++) {
            var aProp = a[props[i]];
            var bProp = b[props[i]];
            if (aProp < bProp) return -1;
            if (aProp > bProp) return +1;  
        }
        return 0;
    };
}
\$\endgroup\$
1
  • \$\begingroup\$ Thanks i only added one line to compare if the properties are equal, i need to be exhaustive if two props are equal i need to check equality in the other props before exiting the function. this is more elegant and really helped me. \$\endgroup\$ Commented Jul 10, 2015 at 17:12
1
\$\begingroup\$

This if construct

if (propA === propB) {
    var array = props;
    array.shift();
    return props.length ? comparePropsOfAWithB(a, b, props) : false
} else if (propA < propB ) {
    return true
} else {
    return false
}

can be written in pseudocode like this

if (condition) {
    return boolean;
} else if (otherCondition) {
    return true;
} else {
    return false;
}  

if the first condition is true, the else if won't be reached so we can rewrite it like

if (condition) {
    return boolean;
} 

if (otherCondition) {
    return true;
} else {
    return false;
}  

now the last if...else if boils down to if otherCondition is true return true otherwise return false. As the otherCondition evaluates to a boolean we can simply return this condition like so

if (condition) {
    return boolean;
} 

return otherCondition;

which leads for your code to

function comparePropsOfAWithB(a, b, props) {
    var prop  = props[0];
    var propA = a[prop];
    var propB = b[prop];

    if (propA === propB) {
        var array = props;
        array.shift();
        return props.length ? comparePropsOfAWithB(a, b, props) : false
    }

    return propA < propB;
}  

which is much simpler and easier to read, but taking into account the answer of @200_success this won't help you, because your assumptions regarding the returnvalue of a compare function is wrong.


You should use descreptive meaningful names for naming variables, methods and classes. You shouldn't use abbreviations for naming because this reduces the readability of your code.

So instead of propA and propB something like firstItemand secondItem or firstProperty and secondProperty would be better.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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