Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style/GuardClause triggered for complex elsif conditions #2737

Closed
amw opened this issue Jan 28, 2016 · 4 comments
Closed

Style/GuardClause triggered for complex elsif conditions #2737

amw opened this issue Jan 28, 2016 · 4 comments

Comments

@amw
Copy link

amw commented Jan 28, 2016

The most recent version of RuboCop as well as current git master trigger this GuardClause errors for my if/elsif/else branches. Example file:

def process_file file
  if file.video?
    puts file.video_info
  elsif file.image?
    puts file.image_info
  else
    raise "unsupported file type"
  end

  puts file.general_info
end

Output:

asd.rb:4:3: C: Use a guard clause instead of wrapping the code inside a conditional expression.
  elsif file.image?
  ^^^^^

I think it doesn't make sense. The guard condition would have to be very complex, the more complex the more elsif clauses there are. Below is – in my opinion – worse code that Rubocop likes:

def process_file file
  raise "unsupported file type" unless file.video? || file.image?

  if file.video?
    puts file.video_info
  elsif file.image?
    puts file.image_info
  end

  puts file.general_info
end

If this looks OK to you then consider the same example, but with 5 elsif checks.

@madwort
Copy link
Contributor

madwort commented Jan 30, 2016

Using case will pass the cop:

def process_file file
  case  
  when file.video?
    puts file.video_info
  when file.image?
    puts file.image_info
  else
    raise "unsupported file type"
  end

  puts file.general_info
end

@alexdowad
Copy link
Contributor

Using case will pass the cop:

That's true, but is the behavior of this cop desirable as re: elsif?

@madwort
Copy link
Contributor

madwort commented Jan 30, 2016

No, I think @amw 's original example is preferable when using elsif

@alexdowad
Copy link
Contributor

This cop already has a clause which catches elsifs, but it does it wrongly.

@bbatsov bbatsov closed this as completed in f05e6f5 Feb 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants