False positive for DuplicateMethodCall with case statement

[gettalong]

Given the following code:

def method
  case x
  when :a
    if y.method || a.method
    end
  when :b
    if y.method || b.method
    end
  end
end

reek reports a DuplicateMethodCall for y.method.

Yes, y.method appears two times in this method, however, it is only called at most once. Therefore I think that this is a false positive.

[mvz]

DuplicateMethodCall isn't intended to be about whether multiple calls actually happen.

In your particular case, y.method || b.method probably has some significance that can be named as a method inside that class:

def method
  case x
  when :a
    if my_condition
    end
  when :b
    if my_condition
    end
  end
end

def my_condition
  y.method || b.method
end

Reek won't report DuplicateMethodCall for this.

[bkuhlmann]

I'm running into this issue as well but with a slight twist on the above. Consider the following:

module Builder
  DEFAULTER = proc { |value| "defaulted to #{value}" }

  def self.call kind, name, default: nil
    case kind
      when :req then name
      when :opt then "#{name} = #{DEFAULTER.call default}"
      when :rest then "*#{name}"
      when :keyreq then "#{name}:"
      when :key then "#{name}: #{DEFAULTER.call default}"
      when :keyrest then "**#{name}"
      when :block then "&#{name}"
      else fail ArgumentError, "Wrong kind (#{kind}), name (#{name}), or default (#{default})."
    end
  end
end

When running Reek on the above, I'll see the following:

[9, 12]:DuplicateMethodCall: Builder#self.call calls 'DEFAULTER.call default' 2 times [https://github.com/troessner/reek/blob/v6.1.0/docs/Duplicate-Method-Call.md]

I think this should be valid because it seems premature (optimization-wise) to compute the default when you might or might not hit the two logic branches that message the defaulter.

[chastell]

I think the point is not to prematurely call the defaulter, but to factor out the call into a method:

module Builder
  def self.call kind, name, default: nil
    case kind
      when :req then name
      when :opt then "#{name} = #{defaulted_to default}"
      when :rest then "*#{name}"
      when :keyreq then "#{name}:"
      when :key then "#{name}: #{defaulted_to default}"
      when :keyrest then "**#{name}"
      when :block then "&#{name}"
      else fail ArgumentError, "Wrong kind (#{kind}), name (#{name}), or default (#{default})."
    end
  end

  def self.defaulted_to value
    "defaulted to #{value}"
  end
end

[bkuhlmann]

Hey Piotr, thanks. Yeah, I see what you are saying but I'm not sure there is much gain in doing this. Plus, what you are suggesting above would result in the same violation since defaulted_to would be detected for the same error (when used as an instance method). My original code snippet wasn't accurate, so here's a revised implementation which demos this better I think:

Instance-Base Implementation

module Marameters
  # Builds a single parameter of a method's parameter signature.
  class Builder
    def initialize defaulter: Defaulter
      @defaulter = defaulter
    end

    # :reek:DuplicateMethodCall
    def call kind, name, default: nil
      case kind
        when :req then name
        when :opt then "#{name} = #{defaulter.call default}"
        when :rest then "*#{name}"
        when :keyreq then "#{name}:"
        when :key then "#{name}: #{defaulter.call default}"
        when :keyrest then "**#{name}"
        when :block then "&#{name}"
        else fail ArgumentError, "Wrong kind (#{kind}), name (#{name}), or default (#{default})."
      end
    end

    private

    attr_reader :defaulter
  end
end

If the calculation of the default was to be refactored out further, you'd loose the elegance of being able to read all possible logic branches from a single source of code and that seems like a shame to split that up in order to satisfy the code smell check.

[gettalong]

I'm closing this since it doesn't seem to be worked on and it seems that the opinions on whether this is actually a problem differs. For myself, I wouldn't factor y.method || a.method out into its own method because the code would get harder to read (it would be different if the condition is longer/harder to read).

[mvz]

@gettalong for us it's useful to have these tickets open and see what people are struggling with. Also, we may change our minds as we gain experience. Finally, reek is a very slow moving project, so something not being worked on doesn't say much, and I'm definitely against closing tickets for being 'stale'.