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 && !is_store
{ status: UNKNOWN_APPLICATION, message: I18n.t('....') }
else
if !is_store
groups = Group.joins(users: :plugins).where(users: {plugins: {found_by => plugin_id}})
.joins(:application_updates)
.where(application_updates: application_id_and_version)
end

if !is_store && 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('....') }
e

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 < 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_fo

StackExchange Code Review Q#113154, answer score: 5