reek
reek copied to clipboard
False positive for DuplicateMethodCall with case statement
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.
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.
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.
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
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.
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).
@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'.