Thoughts on GuardClause

[mockdeep]

We enabled the GuardClause rubocop but found that in some cases the results are disagreeable. I don't know of a good way for rubocop to handle this, but thought I'd see if there were any thoughts. Basically, it would be nice to be able to have nested conditions where there is an error state:

# good
if error_state
  flash[:error] = 'bad state'
  render :new
end

# bad
return unless error_state

flash[:error] = 'bad state'
render :new

But code should not be nested when it is part of the happy flow:

# good
return unless value

user.value = value
user.do_some_magic!

# bad
if value
  user.value = value
  user.do_some_magic!
end

I think one thing that might help is for GuardClause to only check at the beginning of the method for conditionals, though that wouldn't catch everything.

[bbatsov]

I guess the only simple improvement that can be done is restricting the check to the beginning of begin blocks. Outside of that things will get pretty hairy.

[mockdeep]

Yeah, makes sense. We can also eliminate some of the false positives by setting a minimum number of lines, though that will also miss the nested happy states.

[bbatsov]

This is also related to #1238.

[ondrejbartas]

I just run into this issue in rails controllers

class InvoicesController < ApplicationController
  def index
    updated_at = Invoice.order(:updated_at).last.updated_at
    if stale? etag: [updated_at]
      @invoices = Invoice.order(:updated_at)
      respond_with(@invoices)
    end
  end
end

I couldn't find way to refactor code to not throw offense and work.

Can you help me with this?

[mockdeep]

  def index
    updated_at = Invoice.order(:updated_at).last.updated_at

    return unless stale? etag[:updated_at]

    @invoices = Invoice.order(:updated_at)
    respond_with(@invoices)
  end
end

[ondrejbartas]

Thank you very much.
I didn't notice that i need to switch from if to unless :(
My Bad

[alexdowad]

I don't think there is any way for RC to know what is the "good" path and what is the "bad" path. I suggest that this issue be closed. If someone here has a good idea, they can re-open it.