UtilityFunction should not complain method that works inside an interator

[isaiah]

The UtilityFunction smell could be misleading, e.g.

# Just for Fun
class Fun
  attr_accessor :arr

  def bar
    arr.each do |a_variable|
      # This is an intermediate state, should not be communicated as
      # an instance variable, as reek suggests
      b_variable = process(a_variable)
      puts b_variable
    end
  end

  private

  def process(x_variable)
    x_variable + 1
  end
end

reek fun.rb
=> fun.rb -- 1 warning:
=>   [16]:Fun#process doesn't depend on instance state (UtilityFunction)

It is useful most of the time, but for method that process an intermediate state, create a instance variable is not the right way.

[chastell]

I’d argue that Fun#process is a UtilityFunction, but I think we should consider accepting private UtilityFunctions; they’re usually added for readability.

[isaiah]

It is, but instance state doesn't apply here. Actually it's huge NO.

[troessner]

I'm open to changing this because I agree that the current set up of UtilityFunction is too strict.
How about making it part of this smell's configuration if we alert on private UtilityFunction and disable it by default?
That way stricter settings are still possible.

[mvz]

... for method that process an intermediate state, create a instance variable is not the right way.

I think here there is some confusion in the description reek uses: Reek is not trying to suggest changing the method to use an instance variable. In fact, the smell description doesn't try to suggest anything, but one way to resolve this is to move the implementation of process to a_variable's class.

How about making it part of this smell's configuration if we alert on private UtilityFunction and disable it by default?

I'm not too sure: I think UtilityFuntions are in fact often private and not flagging them means we miss out on opportunities to move methods to classes where they belong more naturally.

[andyw8]

@mvz makes a good point. I'd like to see some real-world examples of where this warning would be considered invalid.

[troessner]

@andyw8 what @chastell said above:

I’d argue that Fun#process is a UtilityFunction, but I think we should consider accepting private UtilityFunctions; they’re usually added for readability.

is something I see quite often unfortunately.

[andyw8]

Well let's say that process was a more complex method, and x_variable was a fully-fledged object rather than just a count:

  def process(x_variable)
    if x_variable.code == :foo
      "foo"
    elsif x_variable.code == :bar
      "bar"
    else
      raise "something went wrong"
    end
  end

In that case I think the better action would be to extract this out into a method defined in x_variable's class.

Trying to automatically determine if the method is 'complex enough' to justify this would no doubt be difficult, but I feel that by disabling the warning for private methods, we'd be discarding something which acts as a helpful prompt to refactor in many cases.

[troessner]

Trying to automatically determine if the method is 'complex enough' to justify this would no doubt be difficult, but I feel that by disabling the warning for private methods, we'd be discarding something which acts as a helpful prompt to refactor in many cases.

Agreed - that's why I'd make it configurable ;)

[tuexss]

From the example provided, I'd say this is a misuse of .each do - it looks like avoiding .map, .reduce or .inject, and to do that you need this ugly little helper. Proper use of .each does not need intermediate states to accumulate something across each invocation of the block.

[chastell]

Hmmm. In general moving it to the x_variable’s class would be better (so we should flag it), except when it happends to be a core type (which I’d rather not reopen for this).

We could argue that you shouldn’t use core types too much (as that’s primitive obsession), which I agree with in general, but in practice I do remember factoring out private utility functions to process some data. Factoring out a Process.call(x_variable) private module is just really wrapping the same logic in Process.call() rather than process() – formally probably purer (it blows the smell away!), but not really changes anything other than wrapping the logic in four lines – module + def + end + end – rather than two, def + end.

ArtDecomp::CircuitPresenter::WiresPresenter::WirePresenter::PinPresenter does this and I’ll probably stick with it, but I’m not 100% sure it’s worth it. ;)

[isaiah]

Let's put it the other way, please tell me how the code suggested by reek is acceptable.

# Just for Fun
class Fun
  attr_accessor :arr

  def bar
    arr.each do |a_variable|
      @x_variable = avariable
      b_variable = process
      puts b_variable
    end
  end

  private

  def process
    @x_variable + 1
  end
end

I saw this code in real code base, and it's there basically to conform reek.

[andyw8]

i agree that's a bad approach, but Reek isn't suggesting you do that. There are a number of actions that could have been taken in response to this warning. Choosing the good one requires experience and understanding of OOP – satisfying Reek won't magically turn bad code into good code.

Perhaps the warning could be improved by linking to a deeper discussion of why extracting the behaviour into another object could make sense in some situations.

[isaiah]

Yes, for experienced developers this is not a problem, but for new developers who take the warnings as principle, it does them more harm than good. Maybe besides suggesting turn the parameter passing into instance variable, there could be other suggestions.

[troessner]

Factoring out a Process.call(x_variable) private module is just really wrapping the same logic in Process.call() rather than process() – formally probably purer (it blows the smell away!), but not really changes anything other than wrapping the logic in four lines – module + def + end + end – rather than two, def + end.

Couldn't agree more. Unfortunately, this is exactly what happens in quite a few projects.

Perhaps the warning could be improved by linking to a deeper discussion of why extracting the behaviour into another object could make sense in some situations.

and

Maybe besides suggesting turn the parameter passing into instance variable, there could be other suggestions.

Love the idea!

Do you guys have suggestions for something we can link to? If not, we should come up with something on our own.

However I still believe that we should make it configurable if we enable / disable UtilityFunction for private methods - @mvz, @chastell what are your thoughts on this?

[mvz]

@isaiah:

Yes, for experienced developers this is not a problem, but for new developers who take the warnings as principle, it does them more harm than good.

I think this may be a problem with reek in general: It detects code smells but it doesn't try to tell you what the solution should be. As it stands, it is not really suitable for beginners who don't have a more experienced developer at hand to help guide them toward good OO design.

We can probably do a better job emphasising that the goal should not be to make your code smell free, but more to be aware of problems that may hinder further development and that you may want to tackle if and when you're touching that particular piece of code.

Apart from that, we can provide some sample solutions for the smells in the documentation.

Maybe besides suggesting turn the parameter passing into instance variable, there could be other suggestions.

I'm still not sure where reek suggests this. Can you point that out to me please?

@troessner:

I still believe that we should make it configurable if we enable / disable UtilityFunction for private methods.

I think I'd like to see some more real-world examples before deciding on this. I think most cases of UitilityFunction may be private methods, but I don't know what the conclusion should be. Some threshold on method length may be more appropriate. I'll go over smells in my own code as well and see what the situation is.