Rubocop: Feature Request: Style/EmptyCaseCondition

[shalecraig]

Just ran into an unexpected usage of case statements in ruby,

While this works, it's not the most grok-able thing:

def assert_correct_copy(new_version, old_version)
  case
  when new_version.nil?
    # Weird
    raise "This should never happen -- please pass in non-nil"
  when new_version.to_json == old_version.to_json
    # Do nothing.
    log.info("Doing nothing, new_version == old_version")
  else
    actual_implementation(new_version, old_version)
  end
end

In reality, this picks the first truthy thing.

It'd be cleaner to rewrite as an if-statement:

def assert_correct_copy(new_version, old_version)
  if new_version.nil?
    # Weird
    raise "This should never happen -- please pass in non-nil"
  elsif new_version.to_json == old_version.to_json
    # Do nothing.
    log.info("Doing nothing, new_version == old_version")
  else
    actual_implementation(new_version, old_version)
  end
end

Realistically, I'd love a style cop that checks for usages of case that switch on no variable.

That means:

case foo # <- ok
when bar
# ...

case # <- not ok
when self.can_do_action?
# ...

[rrosenblum]

Interesting. I didn't know that you could write a case statement like that. This seems like a rare case, but it is definitely something that RuboCop could check for.

[shalecraig]

Neither did I! I came across it while reading through some legacy code.

The regex case\n *when works okayish for finding these, but a rubocop's much more elegant than a case statement in this context.

[bbatsov]

Yep, that'd be one useful cop. I believe at some point I planned to write it and add a rule to the style guide about this as well.

[agrimm]

Potential configurability for those who like the reverse: provide an offense for the use of elsif, telling them to use a case statement instead.

[bolshakov]

As for me, case version is much easier to read

[ghost]

I agree with @bolshakov.

This for example:

case
when done?     then :paid
when expired?  then :expired
when expiring? then :expiring
else :pending
end

is better than:

if done?
  :paid
elsif expired?
  :expired
elsif expiring?
  :expiring
else
  :pending
end

[puyo]

Seems very low priority and I can understand both versions, but I do find the elsif version clearer since the empty case is uncommon, it takes me a moment staring at wondering if it's correct, or googling Ruby syntax to double check.

@easybills-admin's example is not a fair comparison. It'd be more like:

case
when done?     then :paid
when expired?  then :expired
when expiring? then :expiring
else :pending
end

if    done?     then :paid
elsif expired?  then :expired
elsif expiring? then :expiring
else :pending
end

Again, to stress, not a priority to me, but I do like the elsif version better.

[jaredk-beta]

FYI - there's an example of an empty case condition in the Ruby style guide: https://rubystyle.guide/#indent-when-to-case.

Personally I don't think the empty case condition deserves an offense, I think both methods are readable if done right.