1
\$\begingroup\$

This function allows me to write this code

const audioOutConfig = new AudioOutConfig();
audioOutConfig.setSampleRateHertz(16000);
audioOutConfig.setEncoding(AudioOutConfig.Encoding.LINEAR16);
audioOutConfig.setVolumePercentage(100);

as this code

const audioOutConfig = config(new AudioOutConfig(), {
  sampleRateHertz: 16000,
  encoding: AudioOutConfig.Encoding.LINEAR16,
  volumePercentage: 100
});

Is this a bad idea, and is the function I wrote to do it acceptable?

function config(obj, opts) {
  for (var opt in opts) {
    if (opts.hasOwnProperty(opt)) {
      obj[`set${opt.charAt(0).toUpperCase() + opt.slice(1)}`](opts[opt]);
    }
  }
  return obj;
}
\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$
const audioOutConfig = new AudioOutConfig();
audioOutConfig.setSampleRateHertz(16000);
audioOutConfig.setEncoding(AudioOutConfig.Encoding.LINEAR16);
audioOutConfig.setVolumePercentage(100);

There's nothing wrong with the original implementation. That's just how it's done.

function config(obj, opts) {
  for (var opt in opts) {
    if (opts.hasOwnProperty(opt)) {
      obj[`set${opt.charAt(0).toUpperCase() + opt.slice(1)}`](opts[opt]);
    }
  }
  return obj;
}

const audioOutConfig = config(new AudioOutConfig(), {
  sampleRateHertz: 16000,
  encoding: AudioOutConfig.Encoding.LINEAR16,
  volumePercentage: 100
});

If you really want to configure it like the above, here's a few things about your implementation. First, config is actually specific to AudioOutConfig. Not everything use set*-like methods. With that, passing in a new AudioOutConfig becomes redundant.

If config can be used on other objects, you risk executing methods that aren't really there. If I put foo:'bar' on the config, it will try calling setFoo('bar') on your object which will fail. You'll need to guard against that.

If you want to stick to the for-in loop, consider inverting the hasOwnProperty check and use continue. That way, there's one less indent in the code.

In the end:

function config(obj, opts) {
  for (var opt in opts) {
    if (!opts.hasOwnProperty(opt)) continue;

    const methodName = `set${opt.charAt(0).toUpperCase() + opt.slice(1)}`
    if(!(methodName in obj)) continue;

    obj[methodName](opts[opt]);
  }
  return obj;
}
\$\endgroup\$
1
\$\begingroup\$

Both the idea and implementation look sound to me. As a nitpick, you might consider using a reduce on the opts, giving you a one liner. Also, I believe the hasOwnProperty test is superfluous here

Untested reduce version

Note that, per Joseph's suggestion, which I agree with, this creates the AudioOutConfig object for you, rather than requiring it as a parameter:

function config(opts) {
  return Object.keys(opts).reduce((audio, opt) => {
    const methodName = `set${opt.charAt(0).toUpperCase() + opt.slice(1)}`
    return audio[methodName] ?  audio[methodName](opts[opt]) : audio
  }, new AudioOutConfig())
}
\$\endgroup\$
4
  • \$\begingroup\$ Since opts is an object, don't have I need to use for..in to iterate? \$\endgroup\$
    – Ari Lotter
    Commented May 16, 2017 at 11:43
  • \$\begingroup\$ you would reduce on the keys of opts, which you can get with Object.keys \$\endgroup\$
    – Jonah
    Commented May 16, 2017 at 16:53
  • \$\begingroup\$ I understand iterating Object.keys, but don't understand how I would use reduce. gist.github.com/arilotter/ff5cdb93c992ac50214742e2a764e4a5 \$\endgroup\$
    – Ari Lotter
    Commented May 16, 2017 at 19:54
  • \$\begingroup\$ I edited the post to include sample code (untested). \$\endgroup\$
    – Jonah
    Commented May 16, 2017 at 20:12

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.