Add helpers for safely checking and setting deep object properties

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
6 years ago
4 years ago

People

(Reporter: sfoster, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [feature] p=0)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
We've got a lot of (if this && this.that && this.that.theOther) guards, and by the looks of the console we need lots more - especially as bindings come unbound and other ephemeral objects cease to be before their properties are accessed.

I've used a getObjectValue(obj, "some.key") and setObjectValue(obj, "some.key", "value") kind of utility on past projects. Something similar on Util might be useful.
(Reporter)

Comment 1

6 years ago
2 Util methods + tests for getting and setting potentially-deep properties using a dot-path like "some.nested.property" for lookup. 

I'm still open to opinions on the actual utility of this, or if it will simply mask errors we would prefer to know about.
Assignee: nobody → sfoster
Attachment #801060 - Flags: feedback?(rsilveira)
Attachment #801060 - Flags: feedback?(mark.finkle)
Comment on attachment 801060 [details] [diff] [review]
Add get/set helpers to avoid null-checks in nested object properties

Review of attachment 801060 [details] [diff] [review]:
-----------------------------------------------------------------

I think these methods are a good addition to util.

::: browser/metro/modules/ContentUtil.jsm
@@ +104,5 @@
> +  },
> +
> +  setValueInObject: function setValue(obj, key, value, shouldCreateAncestors) {
> +    // set a value where the key might be a dot-path
> +    // e.g. setValue(obj, 'foo.bar', "Bar") === obj.foo.bar = "Bar";

nit: In most places I've seen comments describing a method's functionality to be place before the method definition.

@@ +115,5 @@
> +    let lastKey = pathParts.pop();
> +    while ((key = pathParts.shift())) {
> +      if (!(key in obj)) {
> +        if (shouldCreateAncestors) {
> +          obj[key] = {};

This can cause some unexpected side effects but can be useful for creating mock object in tests for example. Glad that shouldCreateAncestors is not the default.
Attachment #801060 - Flags: feedback?(rsilveira) → feedback+
(Reporter)

Comment 3

6 years ago
Yeah creating the ancestors in setValueInObject may just be too evil a potential source of bugs here. I've used it in other projects where it was pretty safe to assume that all ancestor objects were plain old Objects. In this context I can imagine later simple null checks returning true allowing things to fail mysteriously as a values was a object rather than some nsI*, Map or whatever. I think I'll axe that option. Besides, mbrubeck is about to tell me its bad boolean-trap smell anyhow :)
I'm still digesting the use cases. I worry about the performance hit of splitting the string when the code happens to fall in a tight loop, indirectly as functions calling functions can hide things. My OCD want to name the getter a bit differently to convey the context a bit more: maybeGetValueFromObject is a bit long, but the "maybe" conveys why we even want to call this helper. If we were sure, we'd just call it directly.

I was also trying to think of other ways out of this situation. I see several place where we do:

  let someItem = this._getSomeItem();
  if (someItem && someItem.fooBar)
    sendPeopleToMars();

but since the code was refactored a few times, this._getSomeItem now throws instead of returning null and we should fixup the if condition:

  let someItem = this._getSomeItem();
  if (someItem.fooBar)
    sendPeopleToMars();

(In reply to Sam Foster [:sfoster] from comment #3)
> I think I'll axe that option. Besides, mbrubeck is about to
> tell me its bad boolean-trap smell anyhow :)

Yes!
(Reporter)

Comment 5

6 years ago
Ok, I ditched the shouldCreateAncestors option. 

Just yesterday I worked on a patch where I needed 'foo' in bar checks for a few properties. But I hesitated to add a helper like this there out of concern that it was unnecessary abstraction that might obscure what's going on. 

After getting rid of the createAncestors option, the setter doesn't really add much value over an simple assignment. The getter, I find myself needing a 'has' check more than necessarily needing the value of that property. Maybe something like hasProperties('foo', 'bar', 'foo.fooish.fool') or something. And yes, as Tim pointed out, object property names can include periods, so for a general purpose utility, it should probably support 'foo.bar'.baz.brum. Bleagh. My appetite for this helper is suddenly diminished :) 

So, I'm inclined to leave this patch on here as a possibility to be revisited as and when we feel there's a real problem that needs solving.
Attachment #801060 - Attachment is obsolete: true
Attachment #801060 - Flags: feedback?(mark.finkle)
(Reporter)

Comment 6

5 years ago
I'm not working on this currently
Assignee: sfoster → nobody
Blocks: metrobacklog
Whiteboard: [feature] p=0
(Reporter)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.