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

[joshgoebel]

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)

• You could recommend destructuring of the arguments of course - which I would as a mentor.
• Hoisting the constant to a global.

Additional context

None.

[SleeplessByte]

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]

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]

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

[Steffe-Dev]

Hi there @SleeplessByte! I'm keen to make my first PR to Exercism and this seemed like a good option.

1. I agree that adding a helper function here is good practice when the object used to find the number that corresponds to the color is cryptically named (like colors).
2. But, I also agree that a well-named object is just as clear as a helper.

So, I propose that we not change the analysis itself, since it should still continue to catch case (1) above, but rather expand the message to explicitly include commentary on case (2). My working draft changes this:

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

to this:

💬 Using a helper method is good practice, because it replaces a cryptic "member call" with a named call, that can be documented individually. That being said, if the student uses an `Object` with a descriptive name, like `colorToResistance`, leading to code like `colorToResistance[firstColor]`, that's also fine, since the question of clarity posed above would be addressed by the descriptive name.

Let me know if you think this would add value, or if we should opt for a different solution.

[joshgoebel]

I like it.

[SleeplessByte]

Whilst the comment change is a good one @Steffe-Dev, we are no longer generating comments for mentors, and everything is done directly on solutions.

So instead, detecting the code @joshgoebel opened the post with with a message that the const should be hoisted outside of the function is probably the way forward.

[Steffe-Dev]

I see. I can definitely try to implement that. Is there an existing template for the hoisting of variables, or would we have to add one?

Otherwise, I'm still uncertain about what we want to do about the warning relating to the helper method. Because, even if you have the constant as a global, the analyzer would still suggest that you use a helper method instead of indexing the object directly (at least when I run it locally), which I think is what @joshgoebel really wanted to address with this issue. Do we want to keep that warning for now, or should I try to suppress it?

[SleeplessByte]

It should be suppressed. So your options are:

• Does it use an object?
  • If yes, but defined inside, please hoist
  • If yes, and defined outside, all good
• otherwise...
  • usual messages including helper methods

I think there is already logic to see if we are in "array" or "object" mode, so the helper check probably just needs to be _moved.

I think there is already a template to define a top-level constant. That's probably the term you want to look for.

---

Please note that the template content inside this repo is purely for development. The real messages live in website-copy