Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed switch case #4

Closed
wants to merge 2 commits into from
Closed

Conversation

ekologic
Copy link

@ekologic ekologic commented Apr 2, 2018

Using switch cases is not a good practice, in Javascript we can have this beautiful solution to avoid the switch case, it is more readable and extensible

@jonschlinkert
Copy link
Owner

jonschlinkert commented Apr 3, 2018

Using switch cases is not a good practice

Why? I've seen this fallacy repeated numerous times, yet switch statements are often much faster in benchmarks.

edit: It would be helpful if you could link to some documentation or any kind of proof that switch-cases are sub-optimal, or at least describes specific situations in which switch-cases are sub-optimal.

@jonschlinkert jonschlinkert self-requested a review April 3, 2018 00:32
@jonschlinkert
Copy link
Owner

lol i requested a review from myself. I just wanted to see if it would actually work. Fun.

@jonschlinkert
Copy link
Owner

jonschlinkert commented Apr 3, 2018

To run the following code, add it to a file (like bench.js), then do $ npm i benchmark && node bench,

const Benchmark = require('benchmark');
const suite = new Benchmark.Suite();

const types1 = { boolean: true, number: true, string: true, symbol: true, undefined: true };
const types2 = ['boolean', 'number', 'string', 'symbol', 'undefined'];
const values = [
  'foo',
  1,
  function() {},
  {},
  new Date(),
  /foo/,
  Symbol('foo'),
  null,
  undefined,
  void 0,
  true,
  false,
  NaN,
  Infinity
];

suite
  .add('switch', () => values.map(v => isPrimitiveSwitch(v)))
  .add('object', () => values.map(v => isPrimitiveObjectLookup(v)))
  .add('object - in', () => values.map(v => isPrimitiveObjectIn(v)))
  .add('object - own', () => values.map(v => isPrimitiveObjectOwn(v)))
  .add('array', () => values.map(v => isPrimitiveArray(v)))
  .add('equals', () => values.map(v => isPrimitive(v)))

  .on('cycle', event => console.log(String(event.target)))
  .on('complete', function() {
    console.log('Fastest is ' + this.filter('fastest').map('name'));
  })
  .run({ 'async': true });

function isPrimitive(val) {
  if (val === null) {
    return true;
  }
  const type = typeof val;
  return type === 'boolean'
    || type === 'number'
    || type === 'string'
    || type === 'symbol'
    || type === 'undefined';
}

function isPrimitiveArray(val) {
  return val === null || types2.indexOf(typeof val) > -1;
}

function isPrimitiveObjectOwn(val) {
  return val === null || types1.hasOwnProperty(typeof val);
}

function isPrimitiveObjectLookup(val) {
  return val === null || !!types1[typeof val];
}

function isPrimitiveObjectIn(val) {
  return val === null || (typeof val) in types1;
}

function isPrimitiveSwitch(val) {
  switch (typeof val) {
    case 'boolean':
    case 'number':
    case 'string':
    case 'symbol':
    case 'undefined':
      return true;
    default: {
      return val === null;
    }
  }
}

// switch x 3,104,738 ops/sec ±0.61% (90 runs sampled)
// object x 1,947,977 ops/sec ±0.55% (92 runs sampled)
// object - in x 2,160,225 ops/sec ±0.63% (89 runs sampled)
// object - own x 2,376,049 ops/sec ±0.59% (88 runs sampled)
// array x 1,441,834 ops/sec ±0.54% (84 runs sampled)
// equals x 2,983,937 ops/sec ±0.59% (87 runs sampled)
// Fastest is switch

also, fwiw, IMHO the switch statement is far more readable and elegant than the code you're proposing. But my preference for aesthetics doesn't really matter, the performance is what's important.

@rodenmonte
Copy link

rodenmonte commented Apr 3, 2018

Here's another solution that is slightly faster than switch:

function isPrimitiveIf(val) {
  if (val === null
    || typeof val === 'boolean'
    || typeof val === 'number'
    || typeof val === 'string'
    || typeof val === 'symbol'
    || typeof val === 'undefined') {
    return true
  }
  return false
}

Computing typeof val 5 times turns out to be less expensive than assigning the type to a variable. The difference is small though (output of node bench.js w/ a test case added):

switch x 2,405,657 ops/sec ±0.91% (88 runs sampled)
if x 2,512,637 ops/sec ±0.80% (84 runs sampled)
object x 1,507,709 ops/sec ±0.78% (88 runs sampled)
object - in x 1,710,923 ops/sec ±0.90% (86 runs sampled)
object - own x 1,788,273 ops/sec ±1.04% (84 runs sampled)
array x 1,166,770 ops/sec ±0.82% (86 runs sampled)
equals x 2,268,414 ops/sec ±0.67% (85 runs sampled)
Fastest is if

I ran the test a few times on my machine and if was always faster, although it might be different on different machines. This definitely qualifies as a micro-optimization, and is substantially less elegant than the switch-case imo. If you think that this is an improvement, I'll submit a pull to change bench.js (include isPrimitiveIf) and index.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants