Managing many availability status I'm a junior who wants to refactor a big method in my code, which has been tested and works:

  def self.authorized_status plugin_id, store_id, application_id, version, locale=nil
    if plugin_id.match /^(\w|-)+$/
      I18n.locale = locale || :en # I think I can move it to a concern

      # for this I thought about an external method how return two values, or a hash. Any idea?
      found_by = :iid
      plugin = Plugin.find_by_iid(plugin_id)
      if !plugin
        plugin = Plugin.find_by_aid(plugin_id)
        found_by = :aid
      end

      application_id_and_version = { application_id: application_id, version: version, status: Application::PUBLISHED_STATUS }

      update_by_application_id = ApplicationUpdate.where(application_id: application_id).joins(application: :store).where(application: {store_id: store_id} )
      is_store = update_by_application_id.first.application.is_store? if !update_by_application_id.first.nil?

      # for the rest of the code I don't know what to do expect put validations into separate methods. 
      if plugin.nil? || plugin.user.nil?
        { status: UNREGISTERED_PLUGIN, message: I18n.t('....') }
      elsif ApplicationUpdate.where(application_id_and_version).count == 0 &amp;&amp; !is_store
        { status: UNKNOWN_APPLICATION, message: I18n.t('....') }
      else
        if !is_store
          groups = Group.joins(users: :plugins).where(users: {plugins: {found_by =&gt; plugin_id}})
          .joins(:application_updates)
          .where(application_updates: application_id_and_version)
        end

        if !is_store &amp;&amp; groups.empty?
          { status: NOT_AUTHORIZED, message: I18n.t('....') }
        else
          store = Store.find store_id
          authorized = (!store.preauthorized?) || is_store || plugin.authorized_for_store?(store)
          if !authorized || BlacklistedPlugin.find_by_organisation_id_and_plugin_id(store.organisation_id, plugin.id)
            { status: NOT_AUTHORIZED, message: I18n.t('....') }
          else
            { status: AUTHORIZED, message: I18n.t('....') }
          end
        end
      end
    else
      { status: PLUGIN_ID_FORMAT_ERROR, message: I18n.t('....') }
    end
  end


I saw recently to get some help a talk from Andy Pike (slide) with some nice principle like OCP or SOLID. I don't think the way he refactors the code is something I should do but maybe I'm wrong. To be honest, the method sometimes looks so weird, especially with all the ifs, that I don't know where to start.
This method feels like it should be its own class, and the arguments to the method should be constructor arguments. This is really where a "service" class can clean things up, because this method is really doing a lot.

First, I'd like to draw attention to the following lines:


   def self.authorized_status plugin_id, store_id, application_id, version, locale=nil
     if plugin_id.match /^(\w|-)+$/
       I18n.locale = locale || :en # I think I can move it to a concern



The "locale" is being set as a class-level value, which I understand is needed later on. This might have the unintended side affect of changing the locale for outside code. Consider this:

# Change locale to Spanish
I18n.locale = :es

status = YourClass.authorized_status(plugin_id, store_id, application_id, version)

# I18n.locale is now :en


You've now changed the locale for the entire application just by calling this method. In the very least you should save the "current locale" to a local variable, and at the end just before returning the status hash set the locale back to what it was before.

Aside from that, let the refactoring begin!

Given that the return value is a Hash, I'm assuming this is returned from some API controller that converts the Hash to JSON. You have this Business Logic of detecting authorized plugins, which you then couple to the output format of your application back to the client. I would create a class specifically to hold the plugin authorization status:

class PluginAuthorization
  attr_reader :plugin, :status, :message

  def initialize(plugin, is_authorized, status, message)
    @plugin = plugin
    @is_authorized = is_authorized
    @status = status
    @message = message
  end

  def authorized?
    @is_authorized
  end

  def to_h
    { status: status, message: message }
  end
end


This has three advantages:


You are not coupling your Business Logic to your controller's output format
You have a concrete class you could repurpose for a web page as well as a JSON return type
You have a clearly defined interface for just what the authorization data looks like.


Now we can start building our plugin authorization service class -- in fact, that's a perfect name for this class:

class PluginAuthorizationService
  def initialize(plugin_id, store_id, application_id, version)
    # Initialize instance variables
  end

  def authorize(locale = :en)
    # Create and return a new PluginAuthorization object
  end
end


This gives us the basic skeleton to start our refactoring job. Without diving into the gritty details, let's look how you would use this service class in an API controller:

class PluginsController &lt; ActionController::API
  def authorize
    plugin_id = params[:plugin_id]
    store_id = params[:store_id]
    application_id = params[:application_id]
    version = params[:version]
    locale = params[:locale]
    authorization_service = PluginAuthorizationService.new plugin_id, store_id, application_id, version
    authorization = authorization_service.authorize locale

    if authorization.authorized?
      # do something
    else
      # do something else
    end

    render json: authorization.to_h
  end
end


This keeps your controller simple, plus the business logic of authorizing a plugin is encapsulated by the service class. Now that we have an idea what our end goal will be, let's fill in the blanks (and they are big blanks).

First, the status constants should go in the service class, plus let's flesh out the constructor so it does as little work as possible (like any constructor should).

class PluginAuthorizationService

  PLUGIN_ID_FORMAT_ERROR = '...'
  UNREGISTERED_PLUGIN = '...'
  UNKNOWN_APPLICATION = '...'
  NOT_AUTHORIZED = '...'
  AUTHORIZED = '...'

  def initialize(plugin_id, store_id, application_id, version)
    @plugin_id = plugin_id
    @store_id = store_id
    @application_id = application_id
    @version = version
    @application_id_and_version = {
      application_id: application_id,
      version: version,
      status: Application::PUBLISHED_STATUS
    }
  end

  # ...
end


We need a plugin Id, store Id, application Id and version. The original code had the locale default to :en so we can supply that as an argument to the authorize method. The only other thing we do is create a Hash for the @application_id_and_version since this is referenced later as part of the WHERE condition for some queries.

The first bit of cleaning up is the plugin variable, which can be found using one of two methods, with the additional requirement that the method used to find the plugin is used elsewhere in a query:

class PluginAuthorizationService
  # ...

private

  def plugin
    return @plugin unless @plugin.nil?

    @plugin = Plugin.find_by_iid @plugin_id

    if @plugin.nil?
      @plugin_found_by = :aid
      @plugin = Plugin.find_by_aid @plugin_id
    else
      @plugin_found_by = :iid
    end

    @plugin
  end

  def plugin_found_by
    plugin
    @plugin_found_by
  end


Any time you need a Plugin object you call the plugin method. The plugin_found_by method calls plugin to make sure the method by which the plugin is found gets calculated.

Next, we have some ActiveRecord queries to clean up and encapsulate into their own methods:


   # Query 1
   ApplicationUpdate.where(application_id: application_id).joins(application: :store).where(application: {store_id: store_id} )

   # Query 2
   ApplicationUpdate.where(application_id_and_version).count == 0

   # Query 3
   Group.joins(users: :plugins).where(users: {plugins: {found_by =&gt; plugin_id}})
     .joins(:application_updates)
     .where(application_updates: application_id_and_version)



We will create methods on the service class for these queries:

private

  # Query 1
  def application_update
    @application_update ||= ApplicationUpdate.where(application_id: @application_id)
                                             .joins(application: :store)
                                             .where(application: {store_id: @store_id} )
                                             .first
  end

  # Query 2
  def application_update_exists?
    ApplicationUpdate.where(@application_id_and_version).count &gt; 0
  end

  # Query 3
  def plugin_groups
    @plugin_groups ||= Group.joins(users: :plugins)
                            .where(users: {plugins: {plugin_found_by =&gt; @plugin_id}})
                            .joins(:application_updates)
                            .where(application_updates: @application_id_and_version)
  end


Notice that the plugin_groups method calls the aforementioned plugin_found_by method in the WHERE clause for the query.

Next, let's focus on the following line of code in the original post:


   is_store = update_by_application_id.first.application.is_store? if !update_by_application_id.first.nil?



This too is a good candidate for its own method, but first let's modify the ApplicationUpdate class to make it easier to adhere to the Principle of Least Knowledge:

class ApplicationUpdate &lt; ActiveRecord::Base
  delegate :is_store?, :to =&gt; :application, :allow_nil =&gt; true

  # ...
end


This allows us to call application_update.is_store? and not require outside code to traverse the application property. Now, the method on PluginAuthorizationService:

private
  # ...

  def is_store?
    application_update.try :is_store?
  end


Calling application_update.try removes the need for a nil-check.

Another opportunity for properly encapsulated code is in the following line:


   if !authorized || BlacklistedPlugin.find_by_organisation_id_and_plugin_id(store.organisation_id, plugin.id)



The Plugin class should have a method that performs this logic:

class Plugin &lt; ActiveRecord::Base
  # ...

  def blacklisted?(organization_id)
    !!BlacklistedPlugin.find_by_organisation_id_and_plugin_id(organisation_id, self.id)
  end
end


We will also add a few more helper methods to our service class to make our lives easier:

private
  # ...

  def plugin_unregistered?
    plugin.nil? || plugin.user.nil?
  end

  def store
    @store ||= Store.find @store_id
  end

  def valid_plugin_id?
    @plugin_id.match /^(\w|-)+$/
  end

  def authorized?
    @is_authorized ||= !store.preauthorized? || is_store? || plugin.authorized_for_store?(store)
  end

end # End of PluginAuthorizationService class




We've only seen bits and pieces of this, so let's put things all together, and see how the authorize method on the service class ends up:

class PluginAuthorizationService

  PLUGIN_ID_FORMAT_ERROR = '...'
  UNREGISTERED_PLUGIN = '...'
  UNKNOWN_APPLICATION = '...'
  NOT_AUTHORIZED = '...'
  AUTHORIZED = '...'

  def initialize(plugin_id, store_id, application_id, version)
    @plugin_id = plugin_id
    @store_id = store_id
    @application_id = application_id
    @version = version
    @application_id_and_version = {
      application_id: application_id,
      version: version,
      status: Application::PUBLISHED_STATUS
    }
  end

  def authorize(locale = :en)
    previous_locale = I18n.locale
    I18n.locale = locale

    authorization = if !valid_plugin_id?
                      PluginAuthorization.new plugin, false, PLUGIN_ID_FORMAT_ERROR, I18n.t('....')
                    elsif plugin_unregistered?
                      PluginAuthorization.new plugin, false, UNREGISTERED_PLUGIN, I18n.t('....')
                    elsif !application_update_exists? &amp;&amp; !is_store?
                      PluginAuthorization.new plugin, false, UNKNOWN_APPLICATION, I18n.t('....')
                    elsif !is_store? &amp;&amp; plugin_groups.empty?
                      PluginAuthorization.new plugin, false, NOT_AUTHORIZED, I18n.t('....')
                    elsif !authorized? || plugin.blacklisted?(store.organisation_id)
                      PluginAuthorization.new plugin, false, NOT_AUTHORIZED, I18n.t('....')
                    else
                      PluginAuthorization.new plugin, true, AUTHORIZED, I18n.t('....')
                    end

    I18n.locale = previous_locale

    authorization
  end

private

  def application_update
    @application_update ||= ApplicationUpdate.where(application_id: @application_id)
                                             .joins(application: :store)
                                             .where(application: {store_id: @store_id} )
                                             .first
  end

  def application_update_exists?
    ApplicationUpdate.where(@application_id_and_version).count &gt; 0
  end

  def authorized?
    @is_authorized ||= !store.preauthorized? || is_store? || plugin.authorized_for_store?(store)
  end

  def plugin_groups
    @plugin_groups ||= Group.joins(users: :plugins)
                            .where(users: {plugins: {plugin_found_by =&gt; @plugin_id}})
                            .joins(:application_updates)
                            .where(application_updates: @application_id_and_version)
  end

  def is_store?
    application_update.try :is_store?
  end

  def plugin
    return @plugin unless @plugin.nil?

    @plugin = Plugin.find_by_iid @plugin_id

    if @plugin.nil?
      @plugin_found_by = :aid
      @plugin = Plugin.find_by_aid @plugin_id
    else
      @plugin_found_by = :iid
    end

    @plugin
  end

  def plugin_found_by
    plugin
    @plugin_found_by
  end

  def plugin_unregistered?
    plugin.nil? || plugin.user.nil?
  end

  def store
    @store ||= Store.find @store_id
  end

  def valid_plugin_id?
    @plugin_id.match /^(\w|-)+$/
  end

end


The authorize method has been reduced to 22 lines, and by utilizing properly named methods, authorize reads more like natural language and becomes much easier to understand:

if not a valid plugin id
    ...
else if plugin unregistered?
    ...
else if not application_update_exists? and not is_store?
    ...
else if not is_store? and plugin_groups is empty?
    ...
else if not authorized? or plugin blacklisted? by store.organisation_id
    ...
else
    Authorized!
end