In my Rails app I need to implement authentication for web app, I need to use an external resource to make it work. To do so I'm using custom Devise Strategies. After a tremendous amount of work, I finally managed to implement a code that covers all scenarios - the code is working but unfortunately my eyes bleed when I see the code below:

module Devise
  module Strategies
    class AwareLogin < Authenticatable
      def authenticate!
        # some logic
        # (...)

            if login.valid_password?(password) && aware_response.success?
              success!(login)
            elsif login.valid_password?(password) && !aware_response.success?
              success!(login)
            elsif login.id.nil? && aware_response.success?
              login.set_user_tokens(aware_response)
              success!(login)
            elsif !login.valid_password?(password) && !aware_response.success?
              raise ActiveRecord::Rollback
            elsif !login.valid_password?(password) && aware_response.success?
              fail!(:aware_auth)
            end

          rescue SupervisorRollback => s
            @user_to_rollback = s.user_id
            raise ActiveRecord::Rollback
          end
        end
      end
    end
  end
end

Is there any way to replace that if block by something clearer like guard or even maybe external helper instead?

CodePudding user response：

You can consolidate the logic a bit but given that the branches perform different actions you will still need some of the branching.

My recommended consolidation

def authenticate!
  begin
    if login.valid_password?(password) || (set_token = login.id.nil? && aware_response.success?) 
      login.set_user_tokens(aware_response) if set_token
      success!(login)
    else 
      aware_response.success? ? fail!(:aware_auth) : raise(ActiveRecord::Rollback)
    end 
  rescue SupervisorRollback => s
    @user_to_rollback = s.user_id
    raise ActiveRecord::Rollback
  end
end 

Reasoning:

• Your first 2 conditions only differ in their check of aware_response.success?; however whether this is true or false they perform the same action so this check is not needed.
• Third branch performs 1 extra step of setting a token. Since this branch is unreachable unless !login.valid_password?(password) we have simply added an or condition to the first branch to conditionally set the token if this condition is true
• The 4th and 5th conditions can be reduced to an else because we checked if login.valid_password?(password) is true in the first branch thus reaching this branch means it is false. Now the only difference is how we respond to aware_response.success? which I just converted to a ternary.