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

Improve two-fer: recognise re-assignment #52

Open
SleeplessByte opened this issue Sep 26, 2019 · 5 comments
Open

Improve two-fer: recognise re-assignment #52

SleeplessByte opened this issue Sep 26, 2019 · 5 comments

Comments

@SleeplessByte
Copy link
Member

SleeplessByte commented Sep 26, 2019

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. #52 (comment)

@evelynstender
Copy link

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 comment has been minimized.

@evelynstender
Copy link

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

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

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants