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

Cop idea: numbered variable names (user_1 vs user1) #3167

Closed
mockdeep opened this Issue May 26, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@mockdeep
Contributor

mockdeep commented May 26, 2016

We've got a lot of places in our code where we use a couple of records, something like:

user_1 = blah
user_2 = bloo

Sometimes people number them instead, user1 and user2. Would be nice to have a rule to enforce consistent numbering of variable names, so always with _ or always without.

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Jul 3, 2016

Collaborator

Would it also be useful to have a configuration option to disallow numbered variables? I can see how some teams might want this to encourage better design (i.e. using a data structure like Array) and naming conventions.

Collaborator

Drenmi commented Jul 3, 2016

Would it also be useful to have a configuration option to disallow numbered variables? I can see how some teams might want this to encourage better design (i.e. using a data structure like Array) and naming conventions.

@mockdeep

This comment has been minimized.

Show comment
Hide comment
@mockdeep

mockdeep Jul 3, 2016

Contributor

I can't say that it would be something that we would use. We do a lot of testing around things like one-off scripts and database scopes to ensure that only specific records are returned or modified. So something like:

user_1 = create(:user, ...)
user_2 = create(:user, ...)
expect do
  SomeThing.call
end.to change { user_1.reload.some_attribute }
  .and not_change { user_2.reload.some_attribute }
Contributor

mockdeep commented Jul 3, 2016

I can't say that it would be something that we would use. We do a lot of testing around things like one-off scripts and database scopes to ensure that only specific records are returned or modified. So something like:

user_1 = create(:user, ...)
user_2 = create(:user, ...)
expect do
  SomeThing.call
end.to change { user_1.reload.some_attribute }
  .and not_change { user_2.reload.some_attribute }
@mikegee

This comment has been minimized.

Show comment
Hide comment
@mikegee

mikegee Jul 4, 2016

Contributor

I agree with both of you. I think this new cop would have some value and disallowing numbered variables might be the best default config.

Contributor

mikegee commented Jul 4, 2016

I agree with both of you. I think this new cop would have some value and disallowing numbered variables might be the best default config.

@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Jul 4, 2016

Collaborator

There is, of course, a small chance for false positives. Consider, for example, a method call base64 within a class, which would get flagged.

Collaborator

Drenmi commented Jul 4, 2016

There is, of course, a small chance for false positives. Consider, for example, a method call base64 within a class, which would get flagged.

@mikegee

This comment has been minimized.

Show comment
Hide comment
@mikegee

mikegee Jul 4, 2016

Contributor

What if the cop specifically looked for more than one local variable with incrementing suffix numbers? Perhaps starting at 0 or 1?

I think that would catch the problem and not miss any violations.

Contributor

mikegee commented Jul 4, 2016

What if the cop specifically looked for more than one local variable with incrementing suffix numbers? Perhaps starting at 0 or 1?

I think that would catch the problem and not miss any violations.

@mockdeep

This comment has been minimized.

Show comment
Hide comment
@mockdeep

mockdeep Jul 4, 2016

Contributor

It almost sounds like there are multiple rules being discussed here:

  1. (my suggestion) variable names with a number should all be consistently name_1 or name1
  2. variable names should not be named with a number
  3. variable names with numbers should be sequential starting from 1 (or 0)
  4. (another I just thought of) variable names shouldn't have a number if there's only one of them
Contributor

mockdeep commented Jul 4, 2016

It almost sounds like there are multiple rules being discussed here:

  1. (my suggestion) variable names with a number should all be consistently name_1 or name1
  2. variable names should not be named with a number
  3. variable names with numbers should be sequential starting from 1 (or 0)
  4. (another I just thought of) variable names shouldn't have a number if there's only one of them
@Drenmi

This comment has been minimized.

Show comment
Hide comment
@Drenmi

Drenmi Jul 5, 2016

Collaborator

@mockdeep: These would almost certainly go in the same cop (with configuration options), which is why we're discussing them all in the same thread. 😀 Checking adjacent variables is a bit more involved, but might be possible if the rules are well defined.

Collaborator

Drenmi commented Jul 5, 2016

@mockdeep: These would almost certainly go in the same cop (with configuration options), which is why we're discussing them all in the same thread. 😀 Checking adjacent variables is a bit more involved, but might be possible if the rules are well defined.

sooyang added a commit to sooyang/rubocop that referenced this issue Aug 14, 2016

@sooyang sooyang referenced this issue Aug 14, 2016

Merged

[Fix #3167] Added VariableNumber Cop #3416

10 of 10 tasks complete

sooyang added a commit to sooyang/rubocop that referenced this issue Aug 17, 2016

sooyang added a commit to sooyang/rubocop that referenced this issue Aug 17, 2016

sooyang added a commit to sooyang/rubocop that referenced this issue Aug 17, 2016

sooyang added a commit to sooyang/rubocop that referenced this issue Aug 17, 2016

@bbatsov bbatsov closed this in af0bc67 Aug 20, 2016

Neodelf added a commit to Neodelf/rubocop that referenced this issue Oct 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment