4
\$\begingroup\$

I have written a plugin loader in Ruby, and I would like to know if it uses the best technique to handle classes, or if anyone has any recommendations on how to improve my code.

I have written a class called PluginManager, and an accompanying module named Plugin. The names of these are self explanatory, one handles the plugins (loading them), and the other is a module to be included by plugin files.

My Plugin module looks like the following, and currently just replaces the puts method, to ensure all output comes through the logger. I had some issues with making the @@logger variable definable, and I have solved it, but I'm not sure if there is a better way to solve it.

module Plugin
    @@logger = nil

    def puts(string)
        if @@logger.nil?
            super(string)
        else
            @@logger.info string
        end
    end

    # I tried the following, but it didn't work (still said "undefined method 'logger='")

    # class << self
    #   attr_accessor :logger
    # end

    def self.logger=(string)
        @@logger = string
    end
end

My PluginManager class is a bit more complex, but not by much. It simply takes an array of file names, performs some checks to ensure everything exists, then loads them into the loaded_plugins array.

class PluginManager

    def initialize(logger=nil)
        set_logger logger
        @loaded_plugins = []
    end

    def set_logger(logger)
        @logger = logger
        Plugin.logger = logger
    end

    def load(plugins, directory="./plugins/")

        # Iterate through plugins
        plugins.each do |plugin|

            # Work out plugin information
            class_name = plugin.split(".").first.downcase.capitalize
            full_path = directory + plugin
            @logger.info "Loading \"#{class_name}\" from \"#{full_path}\""

            # Ensure the plugin file exists
            if File.file? full_path

                # Load the plugin file and ensure the class exists
                require full_path
                unless eval("defined? #{class_name}")
                    @logger.error "Class \"#{class_name}\" not found!"
                    next
                end

                # Get the plugin's class and ensure it includes plugin
                plugin_class = eval class_name
                if plugin_class.ancestors.include? Plugin

                    # Ensure the initialize method takes no arguments
                    if plugin_class.allocate.method(:initialize).arity > 0
                        @logger.error "\"#{class_name}.initialize\" accepts more than 0 arguments"
                        next
                    end

                    # Load the plugin itself by pushing it into the loaded_plugins list
                    @logger.info "Plugin \"#{class_name}\" loaded!"
                    @loaded_plugins.push plugin_class.new
                    @logger.info "Plugin \"#{class_name}\" enabled!"
                else
                    @logger.error "The class \"#{class_name}\" does not include Plugin!"
                    next
                end
            else

                # Check if the file exists, or is a directory
                unless File.exist? full_path
                    @logger.error "Unable to find file \"#{full_path}\""
                else
                    @logger.error "\"#{full_path}\" is a directory!"
                end

                next
            end
        end
    end
end

I am wondering if there are any better ways to perform some of the checks, such as using exec to find if a class exists.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Formatting

Indenting

The standard ruby indent is 2 spaces. For example:

def puts(string)
  if @@logger.nil?
    super(string)
  else
    @@logger.info string
  end
end

Design

Class Variables

I would not use class variables to set the logging. As you discovered, they have surprising behavior.

Instead, I would just use a class instance variable that belonged just to the Plugin class and was not automatically shared with its descendants.

module Plugin
  class << self
    attr_accessor :logger
  end
end

If that's all you do, then the descendants will have to do this to access the logger:

Plugin.logger.info 'Logging some info'

That's awful, so we'll add an instance method that defers to the class method:

module Plugin

  class << self
    attr_accessor :logger
  end

  def logger
    self.class.logger
  end

end

Now instances of Plugin can log easily:

logger.info "Logging some info"

Ruby tricks

Use #inspect to quote strings

Instead of this,

@logger.info "Loading \"#{class_name}\" from \"#{full_path}\""

Use #inspect to do the quoting for you:

@logger.info "Loading #{class_name.inspect} from #{full_path.inspect}"

Remove eval

You don't need eval here:

unless eval("defined? #{class_name}")

instead:

unless defined?(class_name)

ancestors.include?

Instead of this:

if plugin_class.ancestors.include? Plugin

do this:

if plugin_class < Plugin

See this SO answer, and the rdoc for Module#<

instance_method

Instead of this:

if plugin_class.allocate.method(:initialize).arity > 0

this:

if plugin_class.instance_method(:initialize).arity > 0

This avoids the funny business of allocating one.

exceptions

The code wishes, when a plugin cannot be added, to log it and continue adding more plugins. That works well, but the way it's done can be simplified using exceptions.

Instead of this:

plugins.each do |plugin|
  ...
        unless eval("defined? #{class_name}")
            @logger.error "Class \"#{class_name}\" not found!"
            next
        end

  ...
end

consider this:

class PluginError < StandardError ; end


plugins.each do |plugin|
  begin
    ...
    unless eval("defined? #{class_name}")
      raise PluginError, "Class \"#{class_name}\" not found!"
    end
    ...
  rescue PluginError => e
    @logger.error e.to_s
  end
end

This gets rid of all of those "next" calls, simplifying the error handling.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks for all the help! I'll be working to improve this for some time :) A quick question, you say that instead of if plugin_class.ancestors.include? Plugin, use if plugin_class.ancestors.include? Plugin, which is exactly the same. I assume this is a mistake. What should I use instead of if plugin_class.ancestors.include? Plugin? \$\endgroup\$ Commented Feb 26, 2014 at 23:58
  • \$\begingroup\$ @jackwilsdon Oh dear! It's fixed now. \$\endgroup\$ Commented Feb 27, 2014 at 0:48
  • \$\begingroup\$ What exactly does < do, when referencing classes? Can you link me to the RDoc for this? Thanks. \$\endgroup\$ Commented Feb 27, 2014 at 21:15
  • 1
    \$\begingroup\$ @jackwilsdon Simply, "<" means that the class on the left is derived from, directly or indirectly, the class on the right. "<=" is the same, except that the class on the left may also be the same class as the one on the right. RDOC link added. \$\endgroup\$ Commented Feb 27, 2014 at 22:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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