Code to initialize, set, or get node properties

I have this JavaScript excerpt, and I was wondering if this (primarily the usage of switch case) would be considered good practice. One item of note is that I have it separated into two similar parts to not evaluate the same boolean check over and over again when it is static the whole time.

if (strict) {
  for (var i = 0, len = node.properties.length; i < len; ++i) {
    var other = node.properties[i];
    if (other.key.name !== prop.key.name) continue;
    var otherKind = other.kind;
    switch (kind) { // used as a kind of goto
    case "init":
    case "get":
    case "set":
      if (otherKind === "get" ||
          otherKind === "set" ||
          otherKind === "init")
        raise(prop.key.start, "Redefinition of property");
  }
} else if (sawGetSet)) {
  loop:
  for (var i = 0, len = node.properties.length; i < len; ++i) {
    var other = node.properties[i];
    if (other.key.name !== prop.key.name) continue;
    var otherKind = other.kind;
    switch (kind) { // used as a kind of goto
    case "get":
    case "set":
      if (otherKind === "init") break;
    case "init":
      if (otherKind === "get" || otherKind === "set") break;

    default: continue loop; // skip the below portion, don't throw error
    }
    raise(prop.key.start, "Redefinition of property");
  }
}

Should I rewrite it like this, something else, or leave it as is? (Speed still is important.)

```
if (strict) {
for (var i = 0, len = node.properties.length; i < len; ++i) {
var other = node.properties[i];
if (other.key.name !== prop.key.name) continue;
var otherKind = other.kind;
if ((kind === "init" ||
kind === "get" ||
kind === "set") &&
(otherKind === "get" ||
otherKind === "set" ||
otherKind === "init")) {
raise(prop.key.start, "Redefinition of property");
}
}
} else if (sawGetSet)) {
for (var i = 0, len = node.properties.length; i < len; ++i) {
var other = node.properties[i];
if (other.key.na

For one, I'm wondering about strict vs sawGetSet, and why it's an if...else if structure (instead of just using one of the two variables, and an if...else). But without knowing the rest of the code, I won't get into that.

Anyway, I see no need for switch statements. I don't know if I'd call it "bad practice", but it is certainly strange practice to use a switch when it's not really called for.

I also doubt you gain any appreciable performance by splitting the code into two blocks. At least, I'd advice you to thoroughly benchmark it before duplicating code; premature optimization is the root of all evil, and all that.

As for the logic, it seems you're only interested in 'get', 'set' and 'init' values (I'll call these "the three magic values" in the following), and you call raise() for certain combinations of those. Anything else passes through.

So first, I would move the checking of kind outside the loop, since you can skip everything in your code, if kind isn't one of the three magic values.

Now, if the idea is that kind is guaranteed to always be one of the three magic values, then you can obviously skip that check. But then there's no reason to check its value; you only have to check the other.kind value. So either way, you can skip a bunch of code.

Your code is written to check for kind/otherKind-combinations that'll call raise(), but since that's most combinations, it'd be simpler to turn it inside out, and focus on the combinations that won't call raise().

As far as I can tell, the only configurations that won't call raise() (besides a key.name mismatch) are:

• if kind is not any of the three magic values

• if other.kind is not any of the three magic values

• if

• sawGetSet is true, and

• kind is 'init', and

• other.kind is also 'init'

Everything else will call raise().

So those 3 configurations are the only ones we need to check, and if any of them come up true, we can just skip everything else. If none come up true, call raise().

So from what I can gather, this should have the same result

var i, l,
    kindIsInit, kindIsMagic,
    other, otherIsInit, otherIsMagic;

// we'll be using this more than once
kindIsInit = (kind == 'init');

// Check for configuration #1
kindIsMagic = kindIsInit || (kind === 'get') || (kind === 'set');
if(!kindIsMagic) {
  // I imagine a `continue`, `break` or `return` could go here;
  // I've just put `return` as an example.
  return;
}

for(i = 0, l = node.properties.length; i < l; ++i) {
  other = node.properties[i];
  if(other.key.name !== prop.key.name) {
    continue;
  }

  // we might be using this more than once, too
  otherIsInit = (other.kind === 'init');

  // Check for configuration #3
  // (I'm checking this before #2 because it's probably infinitesimally
  // faster, but, really, the order of 2 & 3 really doesn't matter)
  if(sawGetSet && kindIsInit && otherIsInit) {
    continue;
  }

  // Check for configuration #2
  otherIsMagic = otherIsInit || (other.kind === 'get') || (other.kind === 'set');
  if(!otherIsMagic) {
    continue;
  }

  // If we're here, both kind and other.kind are one of the three "magic"
  // values, and any remaining combination of them should call raise()
  raise(prop.key.start, "Redefinition of property");
}

That should be both faster and simpler than what you have. "Simpler" is of course relative, and you should still copiously document all this, since it'll be tricky to read regardless.

For fun, here's a probably slower (haven't tried it), but more readable (I think) version

// helper function
function parseKind(kind) {
  var isInit = (kind == 'init');
  return {
    init: isInit,
    magic: isInit || (kind === 'get') || (kind === 'set')
  };
}

var i, l, other, kind = parseKind(prop.kind); // I'm guessing `kind` in your code is a shortcut for `prop.kind`.

// Config #1
if(!kind.magic) { return; }

for( i = 0, l = node.properties.length ; i < l ; i++ ) {
  if(node.properties[i].key.name !== prop.key.name) { continue; }

  other = parseKind(node.properties[i].kind);

  // Config #2
  if(!other.magic) { continue; }

  // Config #3
  if(sawGetSet && kind.init && other.init) { continue; }

  raise(prop.key.start, "Redefinition of property");
}

StackExchange Code Review Q#51726, answer score: 6