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

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

Open
joshgoebel opened this issue Oct 7, 2021 · 3 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed x:action/improve Improve existing functionality/content x:knowledge/elementary Little Exercism knowledge required x:module/analyzer Work on Analyzers x:size/small Small amount of work x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises)

Comments

@joshgoebel
Copy link

joshgoebel commented Oct 7, 2021

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
Copy link
Member

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
Copy link
Author

joshgoebel commented Oct 8, 2021

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
Copy link
Member

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

@SleeplessByte SleeplessByte added x:action/improve Improve existing functionality/content x:knowledge/elementary Little Exercism knowledge required x:module/analyzer Work on Analyzers x:size/small Small amount of work x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises) good first issue Good for newcomers help wanted Extra attention is needed labels Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed x:action/improve Improve existing functionality/content x:knowledge/elementary Little Exercism knowledge required x:module/analyzer Work on Analyzers x:size/small Small amount of work x:type/coding Write code that is not student-facing content (e.g. test-runners, generators, but not exercises)
Projects
None yet
Development

No branches or pull requests

2 participants