Assigning class variables smell

[mpapis]

two use cases:

1. assign instance variable directly without a writer (@something = something) - I'm actually not sure about this one

2. assigning instance variable in methods - example:

   attr_reader :something
   def methodA(something)
    @something = something
    methodB
   end
   def methodB
    puts something
   end

   it might be prefered to set up instance variables in initialize so any refactoring would not run the code into missing variables

   stopMichalPapis

[mvz]

Can you elaborate a bit more about why these would be smelly? I think point 2 would be caught by ruby's warnings if @something wasn't assigned yet. Point 1 I think conflicts with the Attribute smell, where we're trying to avoid writers (which I think is the better practice).

[mpapis]

ah so point 1. can be ignored

I did not get any warnings for point 2. if there is an extra work to get those warnings then maybe it's better to also detect it as not everybody would know about it and would rely on reek to provide the best practices not running ruby with extra flags

[troessner]

Just so I get this right:

@mpapis you're voting for:

• using and setting instance variables only in initializers
• when updating them in other methods, writers should be used instead of instance variables

right?

[andyw8]

I read an interesting post on this recently: http://designisrefactoring.com/2015/03/29/organizing-data-self-encapsulation/

[troessner]

Just read it, interesting, thanks @andyw8 !

[troessner]

Well, from what I can see a there are 3 possibilities:

[1] DirectAccess smell [2[ Mixed Access smell [3] SettingInstanceVariablesDirectly smell

Not sure if I would go for [1] and [2], that seems a little overzealous to me. What do you think about [3]?

[mvz]

Yes, that was a very interesting article, @andyw8. I'm partial to Mixed access myself, and I think the problems he shows with adding a last_name setter for Doctor can be avoided by being consistent with Person and setting @last_name in the initializer.

Be that as it may, @troessner, what would SettingInstanceVariablesDirectly detect?

[mvz]

@mpapis, I think Ruby would emit a warning at runtime if you call methodB before methodA. However, I do agree that it's nicer to write the code so this temporal dependency doesn't exist.

So, we don't want, e.g.,

def methodA(something)
  @something = something
  methodB
end

def methodB
  puts @something
end

But we may want to allow:

def initialize
  @something = 'starting value'
end

def methodA(bar)
  @something = calculate_something bar
end
def methodB
  puts @something
end

And I would also want to allow:

  def foo
    @foo ||= calculate_foo
  end

I think this detector would be similar to what ruby-lint does in its undefined variables detector.

[troessner]

I'm partial to Mixed access myself

Me as well.

Be that as it may, @troessner, what would SettingInstanceVariablesDirectly detect?

Basically everything like this:

def omg
  @bar = :hola
end

would smell (including the initializer) whereas

def omg
  self.bar = :hola
end

would be golden.

On second thought, it would be conceptually significantly easier to just prevent DirectAccess at all (That means every "@" would smell per se).

Or maybe this smell doesnt make much sense at all.:)

[mvz]

And that would be to "not set instance variables directly".

I don't understand. Wouldn't that mean not allowing MixedAccess?

[troessner]

I don't understand. Wouldn't that mean not \ allowing MixedAccess?

Yes, you're right and I was rambling.:)

[chastell]

I like this discussion!

I’m not sure whether this will be useful information, but in my personal projects I went from ‘direct instance variable access everywhere’ through ‘accessors everywhere’ to ‘self.foo = bar is ugly, so direct setting of instance variables in the constructor, attr_readers everywhere else’. I don’t mutate my objects unless I have to (I much rather return updated copies instead), but when I do have to I think I use private attr_writers over direct setting the instance variables (so @foo = bar only in the constructor and self.foo = bar outside of it).

[troessner]

I don't think we'll reach a common sense here since it also seems highly subjective. I suggest to close this one. Objections?

[mvz]

Clearly there are a lot of good possible styles here, but perhaps we can still find some actual smells?

I'm thinking @mpapis's smell can be captured by:

• An instance variable must at least be set in the constructor, or be accessed through a method with lazy initialization.

And some more common sense smells:

• Don't access a superclass' instance variables directly in a subclass.
• Don't assume presence of ivars in a module.

[troessner]

This whole discussion has gotten a little vague.

I'm thinking @mpapis's smell can be captured by:

An instance variable must at least be set in the constructor, or be accessed through a method with lazy initialization.
And some more common sense smells:

Don't access a superclass' instance variables directly in a subclass.
Don't assume presence of ivars in a module.

I agree, but then let's rather create separate issues with concrete suggestions.

I'm closing this one.

[mvz]

I'll split this out into 3 discussion issues.

[mvz]

Well, that's 2 issues.