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_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 => 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 > 0
end
# Query 3
def plugin_groups
@plugin_groups ||= Group.joins(users: :plugins)
.where(users: {plugins: {plugin_found_by => @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 < ActiveRecord::Base
delegate :is_store?, :to => :application, :allow_nil => 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 < 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? && !is_store?
PluginAuthorization.new plugin, false, UNKNOWN_APPLICATION, I18n.t('....')
elsif !is_store? && 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 > 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 => @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
