7
\$\begingroup\$

This code is supposed to search for a specific key in a object or an array or a mix between both. Is there anything I can improve?

function getAllObjectsIn(key, data, casesensitive){

    var returns = [];
    var searchForInner = function (key, data, returnParameter, casesensitive){

        if(typeof(data) === 'object'){
            for (var i in Object.keys(data)){
                if(!(casesensitive)){
                    var searchTerm = Object.keys(data)[i].toLowerCase();
                    var key = key.toLowerCase();
                };
                if(searchTerm === key){
                    returns.push(data[Object.keys(data)[i]]);
                }
                else if (typeof(data[Object.keys(data)[i]]) === 'object'){
                    searchForInner(key,data[Object.keys(data)[i]],returnParameter);
                };
            };
        };
    };

    searchForInner(key,data,returns,casesensitive);
    return(returns);
};
\$\endgroup\$
0

1 Answer 1

9
\$\begingroup\$
  • Your function name is not very meaningful in that;

    a) there is nothing to say values returned in the array will be objects (since you are just grabbing whatever data is stored for the key), and

    b) you are not getting "all" data items at all: in fact you are performing a specific key search.

    Perhaps something like recursiveKeySearch() or similar will makes more sense.

  • I also question why you would include case insensitive search behavior against keys in the first place; that is contrary to the expected behavior of key/property names in JavaScript: the potential need to do case-insensitive key/property names evaluations might suggest bad code in other areas of the application where key/property cases are not being applied consistently.

    Don't put in hacks in your code which can make your code perform in unexpected manners.

    If you remove that requirement, you have no need to inspect object.keys anymore and can just directly check if the key exists, greatly simplifying your code.

  • Your code does not really do anything to validate that the data being passed to it is sensible to work with.

    I am guessing that you are really wanting to do is require that data be either an Array specifically, or any other type of object in the Object prototype chain; you have some edge cases you are missing here like typeof(data) === 'object' returning true if data is null.

    When those edge cases happen (which could be often if you are iterating over arbitrary objects) I would think you would still need to handle them within your code.

  • Do you truly wish for you function to be able to search against numeric keys (i.e. for case of Array object)?

    This might seem like a little bit odd of an approach based on JavaScript's inability to actually have numeric keys on an object (they will be converted to strings); this really means that if an integer key is passed, you can only meaningfully search in Arrays, whereas if a string key is passed, you can only meaningfully search in Objects: I personally would expect that this function would not be meaningful for searching numeric Array keys, as typically these keys only indicate "order" not some meaningful value for use in lookup.

    Thus you might consider handling Arrays differently - just iterating and recursing into them, but not actually evaluating them for matches.

    If you need to deep search for numeric key matches, perhaps go with another function altogether.

  • The design of your recursion is a little odd.

    Why the need for an inner function?

  • Your current code approach stops recursion for case where there is a key match; is this really desired?

    I would think expected behavior would be to deep search all values in all keys regardless as to whether the key matched the criteria.

    As it stands, you do not recurse into the value for any cases where you have a key match.

  • You have a few errors in logic around your case sensitivity handling;

    In the case sensitive case, the searchTerm variable would have never been populated, thus your key comparison will always fail.

    In the case-insensitive logic path, you overwrite your key value, potentially breaking your code, as you are now potentially recursing and going through subsequent iterations with a totally new key value.
    This may not actually make a difference with code as written, but I would think you want key to be consistent throughout recursion.

    Also, if you truly want to support integer key values, these make no sense with cases sensitivity, and you would in fact significantly alter the key you are searching for by casting it to string when performing toLowerCase() on it.
    Per my earlier comment around need for case insensitive search, here is a good example for how you have introduced three potential bugs here just because of added complexity on supporting this use case, and not really implementing in best manner.

  • You would be better off setting Object.keys(data) to a variable, so you don't have to keep extracting the keys every time you want to act against them.

    You can obviously avoid this altogether as you should probably simply just replace

This:

for (var i in Object.keys(data)){

with this:

for (var dataKey in data) {

as there is no reason you need to know the (actually meaningless and technically unpredictable) index number around an array created from the key of the data object.

Object properties in JavaScript do not have a guaranteed order, so it seems inappropriate to try to rely on index positions based on this behavior.

Just work with the property names themselves.


Putting it together

If you looked to refactor based on the considerations mentioned above, you might end up with something like:

function recursiveKeySearch(key, data) {
    // not shown - perhaps validate key as non-zero length string

    // Handle null edge case.
    if(data === null) {
        // nothing to do here
        return [];
    }

    // handle case of non-object, which will not be searched
    if(data !== Object(data)) {
        return [];
    }

    var results = [];

    // Handle array which we just traverse and recurse.
    if(data.constructor === Array) {
        for (var i = 0, len = data.length; i < len; i++) {
            results = results.concat(recursiveKeySearch(key, data[i]));
        }
        return results;
    }

    // We know we have an general object to work with now.
    // Now we need to iterate keys
    for (var dataKey in data) {
        if (key === dataKey) {
            // we found a match
            results.push(data[key]);
        }

        // now recurse into value at key
        results = results.concat(recursiveKeySearch(key, data[dataKey]));
    }

    return results;
}

Or, if you REALLY need to add a case insensitive search:

function recursiveKeySearch(key, data, caseSensitive = true) {
    // not shown - perhaps validate key as non-zero length string
    // not shown - perhaps validate caseSensitive as boolean

    // Handle null edge case.
    if(data === null) {
        // nothing to do here
        return [];
    }

    // handle case of non-object, which will not be searched
    if(data !== Object(data)) {
        return [];
    }

    var results = [];

    // determine 'key' value to be used for comparison
    var comparisonKey = key;
    if (caseSensitive === false) {
        comparisonKey = key.toLowerCase();
    }

    // Handle array which we just traverse and recurse.
    if(data.constructor === Array) {
        for (var i = 0, len = data.length; i < len; i++) {
            results = results.concat(recursiveKeySearch(key, data[i], caseSensitive));
        }
        return results;
    }

    // We know we have an general object to work with now.
    // Now we need to iterate keys
    for (var dataKey in data) {
        var isMatch = false;
        if(caseSensitive === false) {
            isMatch = (comparisonKey === dataKey.toLowerCase()); 
        } else {
            isMatch = (comparisonKey === dataKey);
        } 

        if(isMatch) {
            results.push(data[dataKey]);
        }

        // now recurse into value at key
        results = results.concat(recursiveKeySearch(key, data[dataKey], caseSensitive));
    }

    return results;
}
\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much. But you'd have to write results = results.concat(recursiveKeySearch(key, data[i], caseSensitive)); to make your code work. \$\endgroup\$
    – Kastel
    Oct 12, 2016 at 10:31
  • 3
    \$\begingroup\$ @Kastel Indeed. I guess one should always refer back to the documentation when using a function you haven't used in a while :) I have edited. \$\endgroup\$
    – Mike Brant
    Oct 12, 2016 at 13:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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