patternjavascriptMinor
Search in a JSON structure after a key
Viewed 0 times
aftersearchstructurejsonkey
Problem
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);
};Solution
-
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
-
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
-
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
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
In the case-insensitive logic path, you overwrite your
This may not actually make a difference with code as written, but I would think you want
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
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
You can obviously avoid this altogether as you should probably simply just replace
This:
with this:
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-objec
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-objec
Code Snippets
for (var i in Object.keys(data)){for (var dataKey in data) {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;
}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;
}Context
StackExchange Code Review Q#143889, answer score: 9
Revisions (0)
No revisions yet.