The ControlParameter detector is inconsistent.

[dzrw]

I'm not sure why the ControlParameter detector is alerting on the following code (because it looks fine to me), but that it alerts inconsistently seems like a bug. Also, that && notation (used outside of a conditional statement), that's a smell in most languages, but not Ruby?

def make_args(optional_thing = nil)
    args = {
      # ...imagine several key/value pairs...
    }

    # Reek raises a ControlParameter "smell" on the conditional.
    args.merge!('a_key' => optional_thing) if optional_thing

    # But this doesn't raise a smell, even though the logic is identical.
    optional_thing && args.merge!('a_key' => optional_thing)

    # Also, this isn't smelly, apparently...
    case optional_thing
    when nil; nil
    when false; nil
    else
      args.merge!('a_key' => optional_thing)
    end

    # `&& true` is an odor-eater....
    if optional_thing && true
       args.merge!('a_key' => optional_thing)
    end

    args
end

[dzrw]

Passing a hash containing a key used as a control parameter also doesn't raise a smell.

def test(opts = nil)
  opts ||= {}

  if opts['control_param']
    fizzbuzz(opts)
  else
    call_mysql(opts)
  end
end

[troessner]

I'm not sure why the ControlParameter detector is alerting on the following code (because it looks fine to me)

An explanation is given here: https://github.com/troessner/reek/wiki/Control-Couple Frankly, I am not the biggest fan of this SmellDetector as well.

but that it alerts inconsistently seems like a bug.

I agree.

Also, that && notation (used outside of a conditional statement), that's a smell in most languages, but not Ruby?

Oh boy, have I had arguments with Rubyists about that crap, and yes, I totally agree, it IS a big smell.:)

[mvz]

The && notation is common in several languages as an alternative to the if statement. As such, it is standard Ruby idiom (although I tend to prefer the form using and).

I totally agree that ControlParameter is too easily fooled by alternative ways to state the same thing, although some of the alternatives will be deemed smelly for other reasons.

[troessner]

As such, it is standard Ruby idiom (although I tend to prefer the form using and).

But that doesn't imply that it is "best practice" to do so. In my mind this is just misusing an operator that was clearly designed to be used in a guard and not as a means to execute expressions conditionally.

[mvz]

Hm.. I don't mind it so much myself, but that may be my Perl background. The modifier-form of if is probably better, and seems to be recommended by the "The Ruby Programming Language" book.

[dzrw]

I totally agree that ControlParameter is too easily fooled by alternative ways to state the same thing, although some of the alternatives will be deemed smelly for other reasons.

Actually, none of those alternatives raised a smell of any kind.

[mvz]

The version above with the case statement should result in a NilCheck smell.

[gilles-leblanc]

@politician What do you mean by "it alerts inconsistently" ?

[dzrw]

@gilles-leblanc I gave several examples of the code smell that this detector is supposed to flag, but only one narrow form is actually flagged. That implies that this detector isn't really used that much, or that the problem that it's trying to protect us from isn't really a problem (@troessner agrees).

[gilles-leblanc]

@politician Sorry my bad. I was pretty tired when I read it and read over the comments in the code. Everything is pretty clear. Again sorry.

[gilles-leblanc]

I have started working on this.

[troessner]

The referenced PR is merged -> closed.