Two ways of false positives for DuplicateMethodCall

[RST-J]

Hello reekers and thanks for the effort to support a world with better Ruby code.

In my opinion I found two kinds of false positives for the DuplicateMethodCall smell. Consider the following method - which also contains nested iterators and could be split such that the duplicate method calls are not found, but that is not the point. Reek reports duplicate method calls for att[:file_name].

def dedup_attachment_names
  attachments.group_by { |att| att[:file_name] }.each_value do |atts|
    atts.each_with_index do |att, idx|
      next if idx.zero?

      att[:file_name] += "_#{idx + 1}"
    end
  end
end

First, renaming one of the att block arguments makes the duplicate method call going away. My point is, it is a mere syntactical duplication but not a semantic one. It would not be possible to hold the lookup in a separate variable and have it reused in both places.

This is the weaker of my false positives and I'd say as tools like reek have to make tradeoffs its better to get a hint here which makes me reconsider what I wrote and if I'm sticking with it, use a magic comment to disable the finding. Nevertheless I thought, it might be worth to bring that to your attention.

The second false positive, for which I think this should not be flagged, is, that its not actually a duplicate method call. The first is a lookup. The second is a write using +=. I can see the syntactical reason for that, but semantically, to my knowledge, there is no way to avoid that. I see the need for duplicate method calls being flagged with method chaining - foo.bar.baz and foo.bar.baz += 42 should flag foo.bar as duplicate - but I think bar.baz and bar.baz += 42 should not be considered duplicate method calls.

[troessner]

Hi @RST-J and thank you so much for your suggestion.
All of this seems reasonable to me. Unfortunately i am currently swamped so i wont be able to tackle this any time soon. I'll keep this open for sure in case @mvz or somebody is willing to whip up a PR ;)

[mvz]

Taking another look at this because a PR arrived. I find the second false positive less persuasive. The argument is that bar.baz and bar.baz += 42 do not contain a duplicate method call. But if you expand the += you can see that it does:

do_something_with(bar.baz)
bar.baz += 42

becomes:

do_something_with(bar.baz)
tmp = bar.baz
bar.baz = tmp + 42

And the resolution is:

tmp = bar.baz
do_something_with(tmp)
bar.baz = tmp + 42

Whether this is an improvement remains to be seen, of course. But then again, I've come to doubt that for many of the lighter DuplicateMethodCall offenses.