current community

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite
2

I wrote the following code as a way of plucking data from a n-depth JavaScript object. It works a charm and even appends the parents of the the found item.

I was just wondering if you guys had any ideas on making it better.

//#region function recursiveFind
function recursiveFind(data, key, childKey, match, result) {

    ///<summary>Returns JS obj and its parents based on a data set, key and match.</summary>
    ///<param name="data" type="Object" optional="false">the data object you want to find within</param>
    ///<param name="key" type="String" optional="false">the key for the data you want to match against eg. 'id' will look for data.id</param>
    ///<param name="childKey" type="String" optional="false">the data object you want to find within</param>
    ///<param name="match" type="Any" optional="false">the value you wish to search for</param>
    ///<returns type="Object">returns the found result</returns>

    var parents = [];
    var depth = 0;
    var recurse = function (data, key, childKey, match, result) {
        // Build Parents
        depth++;

        // Check this Object
        if (data[key] === match) {
            result = data;
        }
        else if (depth === 1) {
            parents.push(data);

        }

        // If not found check children
        if (result === undefined) {
            if (data[childKey]) {
                $.each(data[childKey], function (k, v) {
                    if (v[key] === match) {
                        result = v;
                        return false;
                    } else {
                        result = recurse(v, key, childKey, match, result);
                        if (result) {
                            parents.push(v);
                            return false;
                        }
                    }
                });
            }
        }

        // Set parents object into the result
        if (result) {
            result.parents = $.extend([], parents);
        }

        // Clean up parents object
        depth--;
        if (depth === 0) {
            parents.length = 0;
        }
        return result;
    };
    return recurse(data, key, childKey, match, result);
}
//#endregion
share|improve this question
 
You you add an example of use? –  plalx Oct 11 '13 at 20:13
 
Are you sure ` parents.length = 0;` works? –  konijn Dec 16 '13 at 19:52
 
@tomdemuyt yep, it is the preferred way of reseting an array in JavaScript stackoverflow.com/questions/1232040/… –  Blowsie Dec 17 '13 at 10:58
add comment

1 Answer

up vote 1 down vote

Overal

I think the code looks too busy just to find a key/value match in an object tree-structure, it took a while to figure out which part(s) bothered me.

Doing it twice

You are doing the matching twice, once with (data[key] === match) and once with (v[key] === match), it would be cleaner to leave the second check to recursion.

Function in a function

I am assuming you use a function in a function to keep track of depth and parents, which isn't worth it. You can do this in a single function with an optional parameter.

Naming

  • match -> matchValue because match sounds like a boolean to me
  • childKey - childrenKey because the key really points to an array
  • result.parents -> avoid this, what if the object already has a property called parents ?

Counter proposal

If you think about it, result can be considered the parent of key, so I would build a function that returns all parents in an array, if you wish you can then have recursiveFind call that.

function findParents( data, key, matchValue, childrenKey, parents )
{
  var i , children , childrenCount;
  /* Prevent the need for an inner function by re-passing parents */
  parents = parents || [];
  parents.push( data )
  /* Return immediately if we find a match */
  if( data[key] === matchValue )
    return parents.reverse(); 
  /* Are there children to inspect ? */
  if( !data[childrenKey] || !data[childrenKey].length )
    return false;
  children = data[childrenKey];
  childrenCount = children.length;
  /* See if any children have it ? */
  for( i = 0 ; i < childrenCount ; i++ )
  {
    if( parents = findParents( children[i] , key , matchValue , childrenKey , parents.slice() ) )
      return parents;
  }
  /* Fail.. */
  return false;
}

Then recursiveFind becomes

function recursiveFind(data, key, childrenKey, match )
{
  var parents = findParents( data , key, match ),
      result = parents.shift()
  result.parents = parents;
  return result;
}
share|improve this answer
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.