Improve two-fer: recognise re-assignment

[SleeplessByte]

Is your feature request related to a problem? Please describe.

When a solution is provided with a re-assignment, the analyzer bails out of a lot of code paths.

export function twoFer(name = 'you') {
  const whomst = name
  return `One for me, and one for ${whomst}.`
}

When a solution is provided with a re-assignment of the named argument, the analyzer doesn't recognise the shadowing:

export function twoFer(name) {
  name = name ? name : "you"
  return `One for me, and one for ${name}.`
}

Which exercise two-fer

Describe the solution you'd like

Simple re-assignments like the one above should be recognised and disapproved. They don't add any value to the solution.

Re-assignments of the named arguments should be recognised and disapproved. https://github.com/exercism/javascript-analyzer/issues/52#issuecomment-750298061

[evelynstender]

Hi @SleeplessByte,

Does this include re-assignments like:

export function twoFer(name) {
  name = name ? name : "you"
  return `One for me, and one for ${name}.`
}

It's re-assigning the param value. I'm asking because I caught a solution like this today. Maybe this should also be recognised and disapproved.

Ignore me if you meant that as well haha

[SleeplessByte]

This is included, but I think this might be recognised already because of the ternary. Do you have a solution id? (URL has it).

[evelynstender]

The id is: 73b697a598c74e3b860500d78cd45312 It suggests that you should use a default param, but I think it should be interesting saying that it's also not good to re-assign a param value.

[SleeplessByte]

Thank you for providing that (it allows me to do two things: find the current message, and run this example through an offline analyzer).

Personally I'm on the fence of giving that as generic advice, but I have no problem with that advice in the context of this exercise. There is an eslint rule called no-param-reassign that protects against function parameter re-assignment because it also modifies the arguments array. It further says:

Often, assignment to function parameters is unintended and indicative of a mistake or programmer error.

I do agree with this. I think often it's by mistake. However, in this case, it'd purposeful and unambigious.

Questions for you (and anyone reading along):

• what is the generic/general advice we would want to give about parameter re-assignment, if any? I think we could go with: "It's often discouraged to re-assign function parameters, because it can be confusing to readers and it mutates the arguments array, which might be unexpected. Use a new variable declaration instead."
• what other reasons would we have that are pointing out (non)-idiomacy (in other words: not style related)?

[evelynstender]

• what is the generic/general advice we would want to give about parameter re-assignment, if any? I think we could go with: "It's often discouraged to re-assign function parameters, because it can be confusing to readers and it mutates the arguments array, which might be unexpected. Use a new variable declaration instead."
• what other reasons would we have that are pointing out (non)-idiomacy (in other words: not style related)?

I think it'll be interesting adding some wording like "it can cause unexpected side-effects" to make sure it's clear and maybe pointing them to The arguments object or even the eslint rule page about it.