3
\$\begingroup\$

I'm using this array to specify the properties I want to operate on. I didn't use for..in because there's a property that doesn't follow the same pattern.

const data = {};

[
  'teaching_levels',
  'axes',
  'accessibility_resources',
  'contents',
  'resources',
].forEach((name) => {
  data[name] = {
    options: results[name],
    values: req.query[name],
  };
});

// This property is different
data.licenses = results.licenses;

Is it alright to have an array like that?

\$\endgroup\$
  • 2
    \$\begingroup\$ I'd like to hear what makes you (OP) think this might not be okay. Any specific concerns? \$\endgroup\$ – Igor Soloydenko Dec 14 '17 at 20:09
4
\$\begingroup\$

I would say, yes, definitely. That saves a lot of manual code writing and makes it easier to change in the event you need to change all of them.

The only difference I would probably make is, add your different properties when you create the object, that will not only slightly improve speed, but will also make the code a tiny bit smaller.

const data = {
    licenses: results.licenses
};

[
  'teaching_levels',
  'axes',
  'accessibility_resources',
  'contents',
  'resources',
].forEach(name => data[name] = {
    options: results[name],
    values: req.query[name],
});
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$

I think I would consider something like this:

const data = {
    teaching_levels: null,
    axes: null,
    accessibility_resources: null,
    contents: null,
    resources: null,
    licenses: results.license,
    decorateResults: function () {
        Object.keys(this).forEach( (prop) => { 
            if (this[prop] === null) {
                this[prop] = {
                    options: results[prop],
                    values: req.query[prop]
                };
            }
        });
    }
}
data.decorateResults();

To me this helps make the data model for data more clear. And decouples the operations of data model definition from data population/decoration more clearly. The init() function could, for example, be an external function. If you want even more decoupling, you could simply pass the data structure to the decorator function, rather than having the decorator function living on the object.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Will it be safe to call this function more than once? (Is it idempotent?) I think, it's not, so I'd prefer this function not to be a part of the object itself, and also name it the way it highlights the fact it is unsafe to call many times. Alternatively, return a new object with the decorated fields, but this changes the way function is used, which may not be desired. So, I like the original code/bear-wrestler's answer better. \$\endgroup\$ – Igor Soloydenko Dec 14 '17 at 22:08
  • \$\begingroup\$ ^ I thought about this initially as well. I think if I were to use this a quick one-off, adding delete this.decorateResults; to the end of the method would suffice. \$\endgroup\$ – I wrestled a bear once. Dec 15 '17 at 3:53
  • \$\begingroup\$ @IgorSoloydenko This is idempotent as you are only writing keys where current property state is null. This state would not exist once initial saturation has occurred. \$\endgroup\$ – Mike Brant Dec 15 '17 at 22:34
  • \$\begingroup\$ @mike you're right. I overlooked the null check. \$\endgroup\$ – Igor Soloydenko Dec 15 '17 at 22:43
  • \$\begingroup\$ @Iwrestledabearonce yes, it would work too, and do the same thing, yet looks dirty solution to me. Similarly, I'm not a big fan of in-place state changes generally, but that's preferential and I don't know the OP's context. \$\endgroup\$ – Igor Soloydenko Dec 15 '17 at 22:50

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.