troessner / reek

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

Initialising instance variables to nil #979

Open troessner opened 7 years ago

troessner commented 7 years ago

Coming from here:

I'd consider initialising instance variables to nil is its own smell. There is no semantic difference between:

class Omg
  def initialize; @bar = nil; end
  def hola(arg); @bar = arg; end
end

and

class Omg
  def hola(arg); @bar = arg; end
end

I think initialising instance variables to nil is a strong hint that.....this shouldn't be an instance variable at all and we should give the user a corresponding suggestion.

WDYT?

mvz commented 7 years ago

Yes, I agree the initialization is irrelevant. I'm not sure what to suggest to the user though, apart from removing the line from the initializer. Perhaps at least adding a (private) reader, since that will absorb uninitialized ivar warnings.

troessner commented 7 years ago

How about suggesting to not make it an instance variable at all?

mvz commented 7 years ago

I think we need to be more specific about the alternatives then :smile:.

troessner commented 7 years ago

But thats what we have a /docs page for, dont we? We can then discuss examples there. We do the same thing for other more complex smells like FeatureEnvy.

mvz commented 7 years ago

Yes, I'm talking about what we should have in the docs.

chastell commented 7 years ago

(FWIW, as I was asked in the other issue: I do agree we should detect this smell and the discussion.) :)

backus commented 7 years ago

Could this awkwardly clash with the fact that ruby emits a warning if you access an ivar before it is initialized? It seems like this is usually the reason for @something = nil to live in initialize

chastell commented 7 years ago

Yes, probably – but I think @something = nil is a smell all the same, and the warning is just a symptom that could be dealt with in better ways (e.g., an @something ||= some_default accessor).