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.

I recently wrote a snippet to make a custom array.indexOf but with nested array support in addition to allow using a nested array as needle.

For example, it correctly returns the index 3 in this case:

var arr1=[0,1,2,[3,[4,true,[],[,"x",false,,,undefined,[]]],["5"]]];
console.log(indexOfArr(arr1,[3,[4,true,[],[,"x",false,,,undefined,[]]],["5"]]));//3

Here is the un-commented snippet (if you want to read the commented version, skip this one):

function indexOfArr(arr1, fnd) {
    for (var i = 0, len1 = arr1.length; i < len1; i++) {
        if (!(i in arr1)) {
            continue;
        }
        if (elementComparer(arr1[i], fnd)) {
            return i;
        }
    }
    return -1;
}

function elementComparer(fnd1, fnd2) {
    var type1 = typeof fnd1;
    var type2 = typeof fnd2;
    if (!((type1 == "number" && type2 == "number") && (fnd1 + "" == "NaN" && fnd2 + "" == "NaN"))) {
        if (type1 == "object" && fnd1 + "" != "null") {
            var len1 = fnd1.length;
            if (type2 == "object" && fnd2 + "" != "null") {
                var len2 = fnd2.length;
                if (len1 !== len2) {
                    return false;
                }
                for (var i = 0; i < len1; i++) {
                    if (!(i in fnd1 && i in fnd2)) {
                        if (i in fnd1 == i in fnd2) {
                            continue;
                        }
                        return false;
                    }
                    if (!elementComparer(fnd1[i], fnd2[i])) {
                        return false;
                    }
                }
            }
        } else {
            if (fnd1 !== fnd2) {
                return false;
            }
        }
    }
    return true;
}

Here the commented snippet:

function indexOfArr(arr1,fnd){

    //compare every element on the array
    for(var i=0,len1=arr1.length;i<len1;i++){

        //index missing, leave to prevent false-positives with 'undefined'
        if(!(i in arr1)){
            continue;
        }

        //if they are exactly equal, return the index
        if(elementComparer(arr1[i],fnd)){
            return i;
        }
    }

    //no match found, return false
    return -1;
}

function elementComparer(fnd1,fnd2){

    //store the types of fnd1 and fnd2
    var type1=typeof fnd1;
    var type2=typeof fnd2;

    //unwanted results with '(NaN!==NaN)===true' so we exclude them
    if(!((type1=="number"&&type2=="number")&&(fnd1+""=="NaN"&&fnd2+""=="NaN"))){

        //unwanted results with '(typeof null==="object")===true' so we exclude them
        if(type1=="object"&&fnd1+""!="null"){
            var len1=fnd1.length;

            //unwanted results with '(typeof null==="object")===true' so we exclude them
            if(type2=="object"&&fnd2+""!="null"){
                var len2=fnd2.length;

                //if they aren't the same length, return false
                if(len1!==len2){
                    return false;
                }

                //compare every element on the array
                for(var i=0;i<len1;i++){

                    //if either index is missing...
                    if(!(i in fnd1&&i in fnd2)){

                        //they both are missing, leave to prevent false-positives with 'undefined'
                        if(i in fnd1==i in fnd2){
                            continue;
                        }

                        //NOT the same, return false
                        return false;
                    }

                    //if they are NOT the same, return false
                    if(!elementComparer(fnd1[i],fnd2[i])){
                        return false;
                    }
                }
            }
        }else{

            //if they are NOT the same, return false
            if(fnd1!==fnd2){
                return false;
            }
        }
    }

    //if it successfully avoided all 'return false', then they are equal
    return true;
}

Things that I want to know:

  • Do you find it readable?
  • Are there any obvious improvements (other than minifying)?
  • What practice is not recommended?
share|improve this question
add comment

1 Answer

Here are some suggestions:

fnd1 + "" == "NaN" would be better as isNaN(fnd1). It will also speed up your code by eliminating those concatenations.

Using the in operator on a array is useless as it checks if the property exists. In the case of an array (which in JavaScript is a special case of an object) you're only testing for the existence of the indexes. Also, continues are almost always unnecessary. For example:

for (var i = 0, len1 = arr1.length; i < len1; i++) {
    if (!(i in arr1)) {
        continue;
    }
    if (elementComparer(arr1[i], fnd)) {
        return i;
    }
}

Could be (I tested this on your fiddle):

for (var i = 0, len1 = arr1.length; i < len1; i++) {
    if (arr1[i] && elementComparer(arr1[i], fnd)) {
        return i;
    }
}

I'm not sure what all that type checking is for as it looks like both parameters should be arrays. Array.isArray should be used.

if (!Array.isArray(fnd1) || !Array.isArray(fnd2) {
    return false;
}

It is always best to use === instead of == as == will do type coercion which can lead to very strange results. Also you should be consistent with the equality operator you decide to use. You occasionally use !== instead of !=.

In terms of readability better parameter names would help as they provide little or no clue about the type they should be. Also, declaring all variable names at the top of your function is a good idea due to JavaScript's hoisting. Most importantly, nesting that deeply is very difficult to follow - especially with multiple returns. It was the hardest part of reading your code. Perhaps a few ifs could be combined.

Lastly, your test array leaves out '', null, and {}. You don't have an object in there at all i.e. {'1': 'foo', bar: 'test'}. Having a number as a property name is very important as it will be true when using in.

share|improve this answer
    
I can't use isNaN() because not only NaN return true (there are more values). Using in is a security check to handle sparse arrays (example). if (arr1[i] && will return to false in many occasions where I don't want (example 0,"",false,null,undefined,NaN) –  ajax333221 Apr 22 '12 at 13:05
    
I am worried about the compatibility of isArray(). I know where can be bugs and where can't on my === == !== == signs. I prefer having a continue break than a nested if –  ajax333221 Apr 22 '12 at 13:12
add comment

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.