troessner / reek

Code smell detector for Ruby
https://github.com/troessner/reek
MIT License
4.03k stars 279 forks source link

Variable assignment to self #1701

Open cswoods6 opened 1 year ago

cswoods6 commented 1 year ago

I was in the situation of replacing multiple method calls with variables and accidentally assigned the variable to itself without any modification. Would these be a useful smell to add?

Example: foo = foo

troessner commented 1 year ago

I think it would be a nice addition - @mvz wdyt?

@cswoods6 if you want to come up with a PR we could certainly guide you towards the goal ;)

mvz commented 1 year ago

Ah yes, I was still going to find out what that code does if this is the first time foo is assigned, and there's a method called foo as well.

It does look weird though so should definitely be caught by either Reek or RuboCop.

JuanVqz commented 11 months ago

should it detect any local variable + method with the same name? even foo = foo is going to be invalid?

troessner commented 11 months ago

So in my mind foo = foo should be a smell regardless if the right hand foo is the same variable or a method. If the right hand foo is a variable its obviously wrong and if its a method then its confusing, particularly if used without the parentheses. If its about saving additional method calls then the left-hand foo should be named differently or it might be smarter to memoize that function call so subsequent calls are "for free".

mvz commented 10 months ago

I just tested this, and this assigns the method result to the variable:

foo = foo()

while this does not and leaves foo nil:

foo = foo

So, I agree the second form is bad and so does RuboCop.

I'm not sure this is really a code smell though? Maybe this should be left to RuboCop instead?