1
\$\begingroup\$

I'm wondering how I can improve my getPath function (I found it on SO and adjusted it to my needs - biggest change was returning an array, used to be a string).

It takes an object and a value as inputs and returns an array of object keys for the first matching value they find, otherwise false.

My focus isn't primarily the speed.
Focus is: As short but at the same time as easy to understand as possible without having to write many comments.

I'm using JS before ECMAScript 6

var nestedObj = {
  "rrr": {
    "ddd": {
      "aa    bbbb": {
        1: 30009463
      },
      "cc    dddd.": {
        1: 30010338
      },
      "e     fff": {
        1: 30007744
      },
      "d-Pool": {
        1: 30018363,
        2: 30017133,
        3: 30013107
      },
      "e   e g": {
        1: 30011707,
        2: 30017137
      },
      "f   f.-f. (f)": {
        1: 30012329
      },
      "g": {
        1: 30011894
      }
    }
  }
};

var result = getPath(nestedObj, 30017137);
document.getElementById("result").innerHTML = result;

function getPath(obj, value, path) {
  if (typeof obj !== 'object') {
    return false;
  }
  for (var key in obj) {
    if (obj.hasOwnProperty(key)) {
      var t = path;
      var v = obj[key];
      //SLICE is super important, otherwise newPath will reference path
      var newPath = path ? path.slice() : [];
      newPath.push(key);
      if (v === value) {
        return newPath;
      } else if (typeof v !== 'object') {
        newPath = t;
      }
      var res = getPath(v, value, newPath);
      if (res) {
        return res;
      }
    }
  }
  return false;
}
<div id="result"></div>

I wrote that one comment as a reminder to myself because I spent too much time wondering why the function didn't work before figuring this out.

\$\endgroup\$
  • \$\begingroup\$ "I'm using JS before ECMAScript 6" - so you are not interested in ECMA 6 / 7 solutions? \$\endgroup\$ – le_m Jun 15 '17 at 22:01
  • 1
    \$\begingroup\$ Here a compact ES7 solution: jsfiddle.net/hjm6pwtv - returns undefined if value can't be found similar to built-in methods. \$\endgroup\$ – le_m Jun 15 '17 at 22:21
  • \$\begingroup\$ Yes, that's what I meant by it - since I want to be able to execute my reviewed code I'm not interested in ECMA 6 / 7 \$\endgroup\$ – Cold_Class Jun 16 '17 at 6:18
  • \$\begingroup\$ Excluding potential other matches would make the program well, less useful, why not return an array of all matches it finds for a specific value? \$\endgroup\$ – Icepickle Jun 16 '17 at 8:24
  • \$\begingroup\$ @Icepickle I thought about that, but this kind of thinking could be spread wider and wider - like why not be able to input an array of values and get an array of possible paths for each given value, etc. - since the method has a very specific purpose in my program currently it isn't less useful this way because my input actually never has the same value twice. But you're very welcome to improve my code in this way, I'm sure it could help people out there (including me in future cases)! :) \$\endgroup\$ – Cold_Class Jun 16 '17 at 10:04
2
\$\begingroup\$

Some thoughts:

  • This is not truly a recursive function, in that you are not passing the path value when first calling the function. This may hint that you should really just recurse privately within the function itself such that you can better control "state" (mainly that of path and whether or not match has been found) and simplify things.
  • You can design away pretty much all of the temporary variables you use within the loop.
  • Why are you making a check for hasOwnProperty? It seems like perhaps you are trying to exclude Arrays, Numbers, etc. here? If that is the case, just check for the object's constructor rather than typeof up front and get rid of this.
  • Consider throwing on case where non-object is encountered in input.
  • Why mix return types? If you are expecting an array as a result, why not have empty array represent "no result" case?

Putting it all together might yield something like:

function getPath(obj, value) {
    if(obj.constructor !== Object) {
        throw new TypeError('getPath() can only operate on object with Object as constructor');
    }
    var path = [];
    var found = false;

    function search(haystack) {
        for (var key in haystack) {
            path.push(key);
            if(haystack[key] === value) {
                found = true;
                break;
            }
            if(haystack[key].constructor === Object) {
                search(haystack[key]);
                if(found) break;
            }
            path.pop();
        }
    }

    search(obj);
    return path;
    /*
    Or alternately if you want to keep mixed return
    return found ? path : false;
    */
}

var nestedObj = {
  "rrr": {
    "ddd": {
      "aa    bbbb": {
        1: 30009463
      },
      "cc    dddd.": {
        1: 30010338
      },
      "e     fff": {
        1: 30007744
      },
      "d-Pool": {
        1: 30018363,
        2: 30017133,
        3: 30013107
      },
      "e   e g": {
        1: 30011707,
        2: 30017137
      },
      "f   f.-f. (f)": {
        1: 30012329
      },
      "g": {
        1: 30011894
      }
    }
  }
};

console.log(getPath(nestedObj, 30017137));
console.log(getPath(nestedObj, 'no value'));

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I mixed return types because this way I could check the outcome simply with if(result) instead of something like if(result.length>0) - is there a downside to that mixed return? - I'm not sure why I needed hasOwnPropertybut I used to get an error sometimes without it so I kept it there over time. I always have a hard time following what is happening in recursion - your search is doing that (truly), right? The if(found) break;is one of those things we need to stop the recursion smoothly once we find the value, right? Without it the result array would probably become very long? \$\endgroup\$ – Cold_Class Jun 16 '17 at 6: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.