2
\$\begingroup\$

As part of a Stack Overflow answer, I whipped up a quick example implementation of a function to set a nested property of an object using a dot notation string. The goal was to show how that could work in a small amount of clean code:

function set (object, property, value) {
  if (typeof property === 'string') property = property.split('.')
  if (property.length > 1) {
    set(object[property[0]], property.slice(1), value)
  } else {
    object[property[0]] = value
  }
}

// Usage:
set(object, 'position.x', 1)

I could have used property.shift() but I wanted to not make the function unnecessarily non-pure and I think that mutating the array would also have made the code harder to follow.

This caused me to get the following comment:

That own-rolled version has potential prototype pollution problems, has a time complexity problem, and can cause a stack overflow. – Ry-

To which I responded:

Can you elaborate on the prototype pollution and time complexity? Is the latter about the repeated slice? Regarding stack overflow it's of course true but we'd have to have an insanely long property path, I'm trading that chance for simplicity.

This version also doesn't check for existence of sub-paths obviously, so yes I could do for example set(something, { length: 123 }, x) or set(something, { slice: 'oh no' }, x) and such and it would crash, but it would also crash on set(something, 'nonexisting.abc', x) at the moment – CherryDT

I then decided to remove the code for the time being and instead open this question here, because I'd be interested to get more feedback about that but the SO question is not the right location for it.

So, I'd be happy to hear your thoughts about it.

EDIT: I realize now, the prototype pollution issue is in case of property being untrusted input, I could write set(something, '__proto__.toString', 'yikes') for example, right? Still, I'd be interested in any other feedback too, now that we are already at it.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Your code looks quite reasonable, there are only a few minor things that stand out to me.

You're constructing a new array every time a recursive call is made. For example, if the property passed is foo.bar.baz.qux.quuz, you will be constructing the following arrays of properties:

['foo', 'bar', 'baz', 'qux', 'quuz'] // initial call to `set`
['bar', 'baz', 'qux', 'quuz'] // 2nd call (recursive)
['baz', 'qux', 'quuz'] // 3rd call (recursive)
['qux', 'quuz'] // 4th call (recursive)
['quuz'] // 5th call (recursive)

These are all entirely separate arrays - when you .slice, you create a new array, it's not just a reference to a particular subset of the old array's indicies. Admittedly, it's unreasonable for a property list to be large enough for this to be a problem, but it seems a bit inelegant for space complexity. If you want to keep going the recursive route, you could fix it by creating the array only once in the initial call, then passing along an index to access.

Since the property is actually expected to be an array of properties, it would make more sense to pluralize the variable name. Also, since you're worried about purity, if you want to make it a bit more functional, rather than reassigning the parameter (which is better avoided when possible), put the result into a different variable, maybe by using a default argument:

function set (object, propertyStr, value, properties = propertyStr.split('.'), i = 0) {
  if (i === properties.length - 1) {
    object[properties[i]] = value;
  } else {
    set(object[properties[i]], '', value, properties, i + 1);
  }
}

const object = {
  position: {
    x: 5
  }
};
set(object, 'position.x', 1);
console.log(object);

But note that the function is still fundamentally impure, because its purpose is to mutate an argument. (That's not a problem unless you want it to be one, just something to keep in mind when discussing purity)

I think the recursion aspect makes things a bit more confusing than they need to be. I'd prefer to use reduce to iterate over the properties and access the last object, then assign the value to the last property on that last object:

const object = {
  position: {
    nested: {
      x: 5
    }
  }
};


function set(object, propertyStr, value) {
  const properties = propertyStr.split('.');
  const lastProperty = properties.pop();
  const lastObject = properties.reduce((a, prop) => a[prop], object);
  lastObject[lastProperty] = value;
}

set(object, 'position.nested.x', 'with pop');
console.log(object);


// If you want to avoid the .pop mutation, then:
function setNoPop(object, propertyStr, value) {
  const properties = propertyStr.split('.');
  const lastObject = properties.slice(0, properties.length - 1)
    .reduce((a, prop) => a[prop], object);
  lastObject[properties[properties.length - 1]] = value;
}

setNoPop(object, 'position.nested.x', 'no pop');
console.log(object);

The code will currently throw an error if an intermediate property doesn't exist or, in strict mode, is not an object. If you want an error to be thrown in this situation, that's OK, but you might prefer for the function to return a boolean indicating success or failure.

const object = {
  position: {
    nested: {
      x: 5
    }
  }
};

const isObj = param => typeof param === 'object' && param !== null;
function set(object, propertyStr, value) {
  const properties = propertyStr.split('.');
  const lastProperty = properties.pop();
  const lastObject = properties.reduce((a, prop) => isObj(a) ? a[prop] : null, object);
  if (isObj(lastObject)) {
    lastObject[lastProperty] = value;
    return true;
  } else {
    return false;
  }
}

const success1 = set(object, 'position.nested.x', 'with pop');
console.log(success1);
const success2 = set(object, 'position.doesNotExist.x', 'with pop');
console.log(success2);

\$\endgroup\$
3
  • \$\begingroup\$ Thank you for the helpful answer! I had some of these thoughts also (but I felt they would make the code more complicated), your opinion helps me valuing them better though. I hadn't thought about the argument mutation as impurity because that new value never leaves the scope of the function, but it seems I misunderstood that. \$\endgroup\$
    – CherryDT
    Commented Apr 21, 2020 at 8:06
  • \$\begingroup\$ Reassigning isn't impure (except in one very weird case with arguments in sloppy mode), but it's non-functional (functional programming and striving for purity usually go hand-in-hand). Even outside of functional contexts, reassigning unnecessarily makes code more difficult to read, and should probably be avoided. \$\endgroup\$ Commented Apr 21, 2020 at 8:32
  • \$\begingroup\$ ...oh I was totally blind now, what you meant is that the mutation of object is impure, and that this is the whole purpose of the function. Rrrrrright. :D \$\endgroup\$
    – CherryDT
    Commented Apr 21, 2020 at 10:33

Your Answer

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

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