3
\$\begingroup\$

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('....') }
          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.

\$\endgroup\$
  • \$\begingroup\$ If the code works then it's indeed on-topic for Code Review. I removed the noise from the noise. Note that while thanking people in advance is nice, we prefer to keep thing simple and concentrated on the question. You can thank answerer by voting on the answer. I hope you'll have good reviews. \$\endgroup\$ – Marc-Andre Dec 7 '15 at 15:24
  • 1
    \$\begingroup\$ What is the name of the class that owns this method? \$\endgroup\$ – Greg Burghardt Dec 7 '15 at 17:32
5
\$\begingroup\$

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:

  1. You are not coupling your Business Logic to your controller's output format
  2. You have a concrete class you could repurpose for a web page as well as a JSON return type
  3. 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
\$\endgroup\$
  • \$\begingroup\$ just whoao. Thanks a lot. This method is alone is one class called PluginAuthorization. Without any injections. I'm gonna try to implement most of your ideas in my code. Just thanks for taking the time. It means a lot for me ! \$\endgroup\$ – Mio Dec 8 '15 at 8:21
  • \$\begingroup\$ Well implemented. It's far better ! Thanks a lot \$\endgroup\$ – Mio Dec 8 '15 at 15:00
  • \$\begingroup\$ This answer is far better than mine, IMHO :) \$\endgroup\$ – Rich Seviora Dec 8 '15 at 19:21
3
\$\begingroup\$

You have a lot of tasks here

  • validate the id
  • retrieve the plugin's record from the database
  • know how you found it (via iid or aid)
  • find the application's record in the database
  • know if the application's a store

... and that's all before you really start checking the plugin record. Once you get to that part, you have some more "tangential" work in querying for a group, or checking the store.

And then you do localization too, which doesn't really belong in what is (presumably) a somewhat deep piece of business logic. It should not also handle user interface.

So, in short, your method does waaaaaay too much.

Other notes:

  • authorized_status isn't a good name. Is a status being authorized? Something simple like authorized_plugin? - or just authorized?, if the method is already on the Plugin class - would be... well, simpler.

    Or call it authorize! and have it raise custom exceptions for non-authorized states. E.g. define some custom exception classes:

    class PluginAuthorizationError < StandardError; end
    class UnregisteredPlugin < PluginAuthorizationError; end
    class UnknownApplication < PluginAuthorizationError; end
    class UnauthorizedPlugin < PluginAuthorizationError; end
    class InvalidPluginId < PluginAuthorizationError; end
    

    and raise them where appropriate. Your calling code can then rescue the different types, or do a catch-all by rescuing PluginAuthorizationError.

    Edit: As Greg Burghardt points out in the comments, using exceptions for flow control (which this basically is) isn't a great solution nor does it necessarily fit the method; it'd either return true or it'd raise an exception, when it should just return true or false. Be sure to check the comments for more.

  • Your id regex is a little strange, as it has a capture group you don't use. Better to use a character class: /^[\w-]+$/? And better yet, use string anchors, not line anchors: /\A[\w-]+\z/.

    Also, it'd be easier to use the =~ operator, i.e. plugin_id =~ /\A[\w-]+\z/.

  • Don't do this

    is_store = update_by_application_id.first.application.is_store? if !update_by_application_id.first.nil?
    
    1. is_store should either be true or false, but here it might end up not being set to anything at all. In practice it's set to nil, which is false'y, but structurally it's still weird.
    2. Don't use if !...; use unless instead.
    3. But actually, don't do either; use #try:

      is_store = update_by_application_id.first.try(:application).try(:is_store?)
      
  • But actually, let's examine that whole query. You're going through ApplicationUpdate, joining Application and Store (which you don't use for anything!) just to find an Application. Something tells me you're going on a big detour there...

  • Don't use a prefix like is_ in method/attribute names. The Ruby way is to say some_app.store?, not some_app.is_store?.

\$\endgroup\$
  • 1
    \$\begingroup\$ The only thing I don't favor is raising exceptions when the authorization fails. To me this falls into using exceptions for flow control --- which has its place. I'm just not sure this is the place. \$\endgroup\$ – Greg Burghardt Dec 7 '15 at 20:00
  • \$\begingroup\$ @GregBurghardt True. Can't say it's a great solution. I was just looking at the original code, which returned constants (well, hashes with constant values). The specificity made me think of exceptions. And it allows for a catch-all, which might be useful. Also it seemed not-too-inappropriate to treat an authorization failure as an exception. But you're still correct. \$\endgroup\$ – Flambino Dec 7 '15 at 20:10
  • \$\begingroup\$ I think if you were marking a plugin as authorized (e.g. saving a value to the database) then throwing exceptions would be appropriate. The OP's code just appears to check if a plugin is authorized. I think it's a difference between telling versus asking. If you ask if the plugin is authorized, a yes/no response is appropriate. Compare that to saying "this plugins is authorized." Throwing an exception is appropriate here to make sure nobody can cheat the system. \$\endgroup\$ – Greg Burghardt Dec 7 '15 at 20:17
  • \$\begingroup\$ @GregBurghardt Again, true. Exceptions are too heavy-handed for this. One could argue that you might just have both; a ? method that rescues the various exceptions and returns false, and a ! method that raises them. Granted, it's not the prettiest thing. In any event, I'll add a note to the answer directing people to the comments \$\endgroup\$ – Flambino Dec 7 '15 at 20:22
  • 2
    \$\begingroup\$ @BeniMio Well, if you go the exception route, you do need to use classes, since that's what exceptions are. However, as Greg pointed out, you may not want to use exceptions. In general though, whether we're talking about exceptions or not, don't be overly reluctant to add classes. Classes are just encapsulations of data and behavior, nothing more. If that makes sense to use, then it makes sense. I mean, in Ruby, Integer is a class - so it's not like making a class is some huge leap by itself. \$\endgroup\$ – Flambino Dec 8 '15 at 9:10
2
\$\begingroup\$

Here's my first-cut refactor. I think there's some things I'm missing here, and doubtlessly could be improved. I do get code smell sense from all the different relationships, but that's out of scope.

class Thing
  def self.authorized_status plugin_id, store_id, application_id, version, locale=nil
    # Early Return Better than Extended Nested If
    return {status: PLUGIN_ID_FORMAT_ERROR, message: I18n.t('....')} unless plugin_id.match /^(\w|-)+$/
    I18n.locale  = locale || :en # I think I can move it to a concern

    # Let's Delegate the Plugin Finding Logic to Another Method
    plugin_match = find_plugin plugin_id
    return {status: UNREGISTERED_PLUGIN, message: I18n.t('....')} unless plugin_match && plugin_match.plugin.user

    # These variables needed more descriptive/concise names.
    app_where_criteria = {application_id: application_id, version: version, status: Application::PUBLISHED_STATUS}
    application_update = ApplicationUpdate.where(application_id: application_id).joins(application: :store).where(application: {store_id: store_id})
    is_store           = application_update.first.application.is_store? unless application_update.first.nil?

    if ApplicationUpdate.where(app_where_criteria).count == 0 && !is_store
      # Specific application Update could not be found, and it is not a store.
      return {status: UNKNOWN_APPLICATION, message: I18n.t('....')}
    end

    unless is_store
      # Is Not a Store Logic
      has_groups = plugin_has_groups?(app_where_criteria, plugin_id, plugin_match.match_method)
      return {status: NOT_AUTHORIZED, message: I18n.t('....')} unless has_groups
    end
    # Is Either a Store or Has Groups
    check_authorization(is_store, store_id)
  end

  def self.check_authorization(is_store, store_id)
    store      = Store.find store_id
    authorized = (!store.preauthorized?) || is_store || plugin.authorized_for_store?(store)
    if !authorized
      {status: NOT_AUTHORIZED, message: I18n.t('....')}
    elsif plugin_is_blacklisted? plugin, store
      {status: NOT_AUTHORIZED, message: I18n.t('....')}
    else
      {status: AUTHORIZED, message: I18n.t('....')}
    end
  end

  # Returns whether the plugin has any groups.
  # @param [Hash] app_where_criteria
  # @param [String] plugin_id
  # @return [Boolean] True if it has any groups, false otherwise.
  def self.plugin_has_groups?(app_where_criteria, plugin_id, found_by)
    Group.joins(users: :plugins).where(users: {plugins: {found_by => plugin_id}})
        .joins(:application_updates)
        .where(application_updates: app_where_criteria).any?
  end

  # Returns a PluginMatch using the plugIn ID provided.
  # @param [String] plugin_id ID for the plugin to be located.
  # @return [PluginMatch] PluginMatch object containing the plugin data.
  def self.find_plugin(plugin_id)
    plugin = PlugIn.find_by_iid(plugin_id)
    if plugin
      return PluginMatch.new(plugin, :iid)
    end
    plugin = Plugin.find_by_aaid(plugin_id)
    if plugin
      return PluginMatch.new(plugin, :aid)
    end
  end

  PluginMatch = Struct.new(:plugin, :match_method)

  # Checks whether the plugin is blacklisted from the store.
  # @param [Store] store to check for the blacklist.
  # @param [PlugIn] plugin to check if blacklisted.
  # @return [Bool] true if blacklisted, false otherwise.
  def self.plugin_is_blacklisted?(plugin, store)
    return !BlacklistedPlugin.find_by_organisation_id_and_plugin_id(store.organisation_id, plugin.id).nil?
  end

end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.