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.

alecxe
17.5k8 gold badges52 silver badges93 bronze badges
asked Jun 15, 2017 at 18:00
\$\endgroup\$
6
  • \$\begingroup\$ "I'm using JS before ECMAScript 6" - so you are not interested in ECMA 6 / 7 solutions? \$\endgroup\$ Commented Jun 15, 2017 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\$ Commented Jun 15, 2017 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\$ Commented Jun 16, 2017 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\$ Commented Jun 16, 2017 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\$ Commented Jun 16, 2017 at 10:04

1 Answer 1

3
\$\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'));

answered Jun 15, 2017 at 21:12
\$\endgroup\$
1
  • \$\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\$ Commented Jun 16, 2017 at 6:36

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.