1
\$\begingroup\$

Banner

Some time ago I started with a small Ruby project. I call it social_aggregator. This software aggregates information from different networks to one xml-stream, which you can reuse. For instance on your personal website to show some of your activity to your audience or as your personal news feed.

I've written something like a plugin interface and a manager, which loads the plugin during the boot up process. A plugins aggregates data from a social network or another data source. One plugin per data source.

There is a PluginFrame.rb, which provides the interface and generalize some plugin functionality:

require 'celluloid'
require 'digest/md5'

require 'app/models/Plugin'
require 'app/models/Action'
require 'app/models/Log'
require 'app/models/Message'
require 'app/models/MessageCategory'
require 'app/models/Follower'

require 'app/utils/Logging'
require 'app/utils/Setting'

class PluginFrame
    include Setting
    include Logging
    include Celluloid

    finalizer :unload

    def initialize(plugin_model)
        @plugin = plugin_model

        settings_path @plugin.conf_path

        logger.info "The plugin #{@plugin.name} has been initialized."
    end

    def run
        logger.warn "The plugin #{@plugin.name} is not implemented!"
        terminate
    end

    def unload
        logger.warn "The plugin #{@plugin.name} is terminating!"
    end

    protected

    # Returns a persisted action by given name
    def get_action(name)
        Action.find_or_create_by!(name: name, plugin: @plugin)
    end

    # Return whether last action occurance was before given time
    def action_ready?(action, timer)
        time_since_last_occurance = action.last_occurance

        unless time_since_last_occurance.nil? || time_since_last_occurance > timer
            logger.info "Possible aggregation in #{timer - time_since_last_occurance} seconds."
            return false
        end
        return true
    end
end

The PluginManager.rb checks if a plugin is valid and loads them:

require 'celluloid'

require 'app/models/Plugin'

require 'app/utils/Logging'
require 'app/utils/Setting'

require 'app/plugins/PluginValidator'
require 'app/plugins/PluginWorker'

class PluginManager
    include Logging
    include Setting
    include Celluloid

    # Stores all valid plugin models
    @plugin_definitions

    # Stores all plugin instances (threads)
    @plugin_instances

    def initialize
        logger.info 'Initializing plugin manager'

        @plugin_definitions = []
        @plugin_instances = []

        initialize_plugins
    end

    def defined_plugins
        @plugin_definitions
    end

    def loaded_plugins
        @plugin_instances
    end

    def run
        logger.debug 'Aggregating data from plugins.'

        if loaded_plugins.count <= 0
            logger.info 'No plugins loaded to aggregate data from.'
            Aggregator::shutdown
            return
        elsif loaded_plugins.count > 2
            pool_size = loaded_plugins.count
        else
            pool_size = 2
        end

        plugin_worker = PluginWorker.pool(size: pool_size)
        loaded_plugins.map { |p| plugin_worker.future.run(p) }.map(&:value)
    end

    private

    def initialize_plugins
        plugins = []
        logger.info 'Initializing plugins'

        search.each do |p|
            plugin = PluginValidator::validate p

            unless plugin.nil?
                plugins << plugin
            end
        end

        logger.info "Found #{plugins.count} valid plugins."

        plugins.each do |p|
            plugin = Plugin.find_or_initialize_by(name: p.name)

            plugin.update_attributes(
                class_name: p.class_name,
                conf_path: p.conf_path,
                class_path: p.class_path
            )

            @plugin_definitions << plugin
        end

        logger.info 'Persisted plugin information.' if plugins.count > 0

        @plugin_definitions.each do |p|
            begin
                require p.class_path
            rescue => e
                logger.warn "Couldn't parse file #{p.class_path}. Aggregator is not able to use the #{p.name} plugin."
                logger.debug e
                next
            end

            begin
                if Object::const_get(p.class_name).ancestors.include? PluginFrame
                    instance = Object::const_get(p.class_name).spawn(p)
                    @plugin_instances << instance
                    logger.info "Plugin #{p.name} initialized"
                else
                    raise
                end
            rescue => e
                logger.warn "Couldn't instantiate class #{p.class_name} or class is not a plugin. Aggregator is not able to use the #{p.name} plugin."
                logger.debug e
            end
        end

        logger.warn 'Found no useable plugin!' if @plugin_instances.empty?
    end

    # Search for plugins
    def search(directory = setting.plugin_folder)
        plugins = Dir.glob("#{directory}/**")

        if Aggregator::environment == :development
            plugins.each do |p|
                logger.debug "Found plugin folder #{p}. Validating plugin now."
            end
        end

        plugins
    end
end

My questions are:

  1. How would you modularize the functionality to pull the data from different sources?
  2. What about to put the plugin into gems?

If you're interested in the project, you can find it here: https://github.com/openscript/social_aggregator

\$\endgroup\$
2
\$\begingroup\$

Most of your code is not here, but some general points:

  1. File naming conventions: in ruby, file name convention is snake case - your files should be named as the name of your class, but as snake case - meaning plugin_frame.rb, plugin_manager.rb, message_category.rb. Remember to change you requires also.
  2. Finalizers are rarely if ever used in ruby, and I see no compelling reason to use them in your code.
  3. Ruby is a duck typed language - you don't have to declare methods just to say that they are not implemented - simply don't implement them.
  4. If you don't want to allow the initialization of the class PluginFrame you can refactor it to be a module. A module cannot be initialized by itself, but classes which include inherit its methods.
  5. You should abstract the use of members in your code - don't use @plugin, instead dd an attribute_reader :plugin and use plugin.
  6. If you are using rails you don't need to explicitly require the models, they should be automatically loaded when the server starts
  7. Refrain from using unless when there are action both for the positive and the negative results - if is much more readable
  8. Ruby is intended to more compact, so if the return value should be true or false due to some condition, simply return the result of the condition (no need for return true). Even the return keyword is not needed.

Refactored plugin_frame.rb:

require 'celluloid'
require 'digest/md5'

require 'app/utils/logging'
require 'app/utils/setting'

class PluginFrame
  include Setting
  include Logging
  include Celluloid

  protected

  attr_accessor :plugin

  # Returns a persisted action by given name
  def get_action(name)
    Action.find_or_create_by!(name: name, plugin: plugin)
  end

  # Return whether last action occurrence was before given time
  def action_ready?(action, timer)
    time_since_last_occurrence = action.last_occurrence

    time_since_last_occurrence && time_since_last_occurrence <= timer
  end
end

This class is much more readable, succinct, and more easily shows its usage and responsibility.

Unfortunately, this class seems to contain some partial responsibility - reading an action, checking if its ready, but it misses other actions which are assumed to be handled else-where - setting action's last occurrence and saving its state, for example. I assume this is written in some other class, but it breaks the Encapsulation guideline - put the logic for loading and saving an object in the same place, evaluating and maintaining state in the same place, etc...

As for the plugin_manager.rb, some more points:

  1. You don't need to declare members in your class (@plugin_definitions, etc.). When they are initially assigned they 'magically' appear.
  2. You define getters, which are fine, although you can use attr_accessors and attr_readers instead. Any way, you give your getters a different name than the members. This is not advisable - give them the same name.
  3. Instead of initializing an array, then using each on another array, and pushing the results to the first array, simply use map and select.
  4. Method naming - if you feel that a method needs a comment (namely search) to explain what it does, it might be better to rename it to be self-explanatory.

Refactored:

class PluginManager
  include Logging
  include Setting
  include Celluloid 

  def initialize
    initialize_plugins
  end

  attr_reader :plugin_definitions, :plugin_instances

  def run
    logger.debug 'Aggregating data from plugins.'

    if plugin_instances.count <= 0
      logger.info 'No plugins loaded to aggregate data from.'
      Aggregator::shutdown
      return
    end

    pool_size = [plugin_instances.count, 2].max   
    plugin_worker = PluginWorker.pool(size: pool_size)
    plugin_instances.map { |p| plugin_worker.future.run(p) }.map(&:value)
  end

  private

  def initialize_plugins
    logger.info 'Initializing plugins'    

    plugins = search_for_plugins.map { |p| PluginValidator::validate p }.compact   
    logger.info "Found #{plugins.count} valid plugins."

    @plugin_definitions = plugins.map do |p|
      Plugin.find_or_initialize_by(name: p.name).tap do |plugin|
        plugin.update_attributes(
          class_name: p.class_name,
          conf_path: p.conf_path,
          class_path: p.class_path
        )
      end
    end    
    logger.info 'Persisted plugin information.' if plugins.count > 0

    @plugin_instances = plugin_definitions.map do |p|
       begin
         require p.class_path
         begin
           if Object::const_get(p.class_name).ancestors.include? PluginFrame
             Object::const_get(p.class_name).spawn(p)
           else
             raise
           end
         rescue => e
           logger.warn "Couldn't instantiate class #{p.class_name} or class is not a plugin. Aggregator is not able to use the #{p.name} plugin."
           logger.debug e
         end
       rescue => e
         logger.warn "Couldn't parse file #{p.class_path}. Aggregator is not able to use the #{p.name} plugin."
         logger.debug e
       end
     end

     logger.warn 'Found no useable plugin!' if plugin_instances.empty?
  end

  def search_for_plugins(directory = setting.plugin_folder)
    Dir.glob("#{directory}/**").tap do |plugins|
      if Aggregator::environment == :development
        plugins.each do |p|
          logger.debug "Found plugin folder #{p}. Validating plugin now."
        end
      end
    end
  end
end

To package your code as a gem you can start by reading the guide

As for separating the plugins into separate gems - that is mostly a matter of opinion, I would advise against that - implement your plugins in the main gem, which then can be used as reference implementations for your gem users, which will be able to develop their own plugins in their applications, or within their own gems.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much for this great review. I've adapted almost everything and I learnt something from you. I've one more question and I believe you misunderstood my question with the gems. Would you put my plugins, which are managed by the plugin_manager you reviewed , into separate gems? Does this makes any sense? \$\endgroup\$ – Robin Feb 19 '14 at 18:10
  • 1
    \$\begingroup\$ I've updated my answer \$\endgroup\$ – Uri Agassi Feb 19 '14 at 18:25

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.