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

[SleeplessByte]

@Steffe-Dev if you need any more help or pointers, ping me here or on the Exercism Discord

[Steffe-Dev]

Great, thank you so much for all the patience in explaining everything! I will let you know if I need anything else. 👌

[Steffe-Dev]

@SleeplessByte So, after hacking away at this for a while, I realised that we have two distinct problems to solve:

1. Detect if a constant was defined inside a method, and not at the top level of the file. This can be an array or an object.
2. If we have a top level object defined, we should not bother with comments about helper methods.

When you inspect the code inside ResistorColorDuoSolution.ts, especially isOptimalMathSolution, you will see that the helper method's existance is deeply entwined with many of the other math solution checks, which means a more significant refactor would be needed to properly separate the helper method logic from the other logic to such an extent that the helper method check can be bypassed independently. This means that (2) potentially has a good deal of scope creep.

My proposal is that we start by solving (1) in its own PR, which is a self-contained check that can be inserted into the various optimal check methods, which does an early return before the helper method checks, and would thus address @joshgoebel specific request (with the source file at the top of this issue) that the error message be replaced by something more useful (the PREFER_EXTRACTED_TOP_LEVEL_CONSTANT template).

Then we can re-evaluate the path forward of this issue. Let me know what you think, if there is a better option then I am open. 😄

[SleeplessByte]

Yah sounds fine with me!

You could also check if the TS analyzer has a better base here with less intertwining

[Steffe-Dev]

@SleeplessByte Unfortunately the TS analyzer only has Two Fer at the moment 😅

I created a PR for what I described above, but the bot automatically closed it. I'm still learning your processes, so I don't know how to avoid that, and I would appreciate help with getting it open again. Once open, there is also a pending commit that will come through that removes an unused variable that I initially forgot about. Thanks in advance 😄

[SleeplessByte]

All good. We manually reopen it if there's merit. Which there is!!!