exercism / javascript-analyzer

This is Exercism's automated analyzer for the JavaScript track.
GNU Affero General Public License v3.0
16 stars 16 forks source link

Incorrect Analysis: JavaScript analyzer makes little sense when a object literal is used #127

Open joshgoebel opened 2 years ago

joshgoebel commented 2 years ago

Describe the incorrectness

I don't think the analyzers advice makes any sense here:

Using a helper method is good practice, because it replaces a cryptic "member call" with a named call that can be documented individually.

The advice assumes a student is using an array - in which case there is complexity to hide and I agree a function as a named abstraction is useful, but in this case a well-named lookup table is serving the same purpose as a function, so the advice makes little sense.

Which exercise

Resistor Duo

Source file(s)

export const decodedValue = (colors) => {
  const resistorBands = {
    black: 0,
    brown: 1,
    // ...
    grey: 8,
    white: 9
  }
  return resistorBands[colors[0]]*10 + resistorBands[colors[1]];

};

Expected analysis

None. (well at least not on this point)

Additional context

None.

SleeplessByte commented 2 years ago

I do think that:

function colorValue(color) {
  return resistorBands[color]
}

is more idiomatic and readable than

resistorBands[firstColor]

...but I do agree it's less problematic.

joshgoebel commented 2 years ago

But really that's a just naming concern, IMHO. (I personally think colorValue is not great naming here) The lookup table should be named such as to fully indicate its purpose.

So I don't see a significant difference in expressiveness between:

// object literal
COLOR_to_RESISTANCE[tensColor] * 10 + COLOR_to_RESISTANCE[onesColor]

// function dispatch
colorToResistance(tensColor) * 10 + colorToResistance(onesColor)

Wrapping the object literal in a function dispatch just starts to feel like needless abstraction to me - and I've certainly steered students aways from such needless wrapping in other cases for sure.

I don't feel the auto-intelligence should flag either of the above solutions.

SleeplessByte commented 2 years ago

That's a good case. Ok. I agree with your assessment :)