3
\$\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 a built in Webrick server, while I also want to support any other rack based server. So I have this two entry/start points:

config.ru:

$LOAD_PATH.unshift(File.dirname(__FILE__))

require 'Aggregator'
require 'conf/router'

# Run aggregator
app = Thread.new{ Aggregator.new }
app.run

use ActiveRecord::ConnectionAdapters::ConnectionManagement
use ActiveRecord::QueryCache

run Router.map

And the Aggregator.rb:

$LOAD_PATH.unshift(File.dirname(__FILE__))

require 'conf/initializers/DatabaseInitializer'
require 'conf/initializers/ConsoleInitializer'
require 'app/plugins/PluginManager'
require 'app/utils/ArgumentParser'
require 'app/utils/Logging'
require 'app/utils/Setting'

# Main class
class Aggregator
    include Logging
    include Setting

    # The version constant
    AGGREGATOR_VERSION = '0.0.1' unless const_defined?(:AGGREGATOR_VERSION)

    # Stores the environment
    @@environment = :production

    # Stores the run state
    @@stop = false

    # Initialize the whole system
    def initialize(internal_server = false, arguments = [])
        options = ArgumentParser.parse(arguments)

        Logging::environment options.environment

        # Set up logger
        if options.quiet
            Logging::quiet = true
        end

        logger.info 'Starting up aggregator now'

        # Set environment to setting
        logger.info "Using #{options.environment} environment"
        @@environment = options.environment

        # Connect database with orm
        DatabaseInitializer.new(options.environment)

        unless options.environment == :test || options.console
            # Set up plugin manager
            @@plugin_manager = PluginManager.new 

            if internal_server
                require 'conf/initializers/ServerInitializer'

                # Spawn new server
                ServerInitializer.new
            end

            logger.info "Aggregator is up and running"

            Signal.trap("SIGINT") do
                Thread.current.exit
            end

            start
        end

        if options.console
            ConsoleInitializer.new(options.environment)
        end
    end

    # Returns the current environment
    def self.environment
        @@environment
    end

    # Returns the version number
    def self.version
        AGGREGATOR_VERSION
    end

    # Returns the plugin manager
    def self.plugin_manager
        @@plugin_manager
    end

    # Shutdown the system
    def self.shutdown
        @@stop = true
    end

    private

    # Starts the aggregation
    def start
        @@plugin_manager.run

        if @@stop
            logger.info "Stopping aggregation now, due request to stop."
            return
        end

        logger.debug "Aggregation done. Next aggregation in #{setting.aggregate_timer} seconds."

        sleep setting.aggregate_timer
        start
    end
end

# Initialize aggregator
if __FILE__ == $PROGRAM_NAME
    app = Aggregator.new(true, ARGV)
end

Is that a good way to realize a built in server as well as support for other app server? If you're interested in the project, you can find it here: https://github.com/openscript/social_aggregator

\$\endgroup\$
1
\$\begingroup\$

You can see the answer to Implementing plugins in my Ruby social aggregator app to see some general observations on your code style.

The most important thing I have to point out on this code is that your run loop is not a loop, but a recursion. This is especially bad, since it will eventually kill your whole application on a StackOverflow exception (pun?)

You should always use iteration over recursion when the loop is infinite:

def start
  until @@stop
    @@plugin_manager.run

    logger.debug "Aggregation done. Next aggregation in #{setting.aggregate_timer} seconds."

    sleep setting.aggregate_timer
  end
end

Also, you seem to mix instance scope and class scope. You allow for multiple Aggregators, but there is only a single shutdown. Either design your class for multiple uses, in which case state and methods should be of the instance, or that it is a Singleton, for which there are patterns in ruby.

You seem to have a lot of comments in your code. Ruby encourages less comments, and more expressive method names. There is no point in a comment, if it does not add anything to the code itself. This code:

# Initialize aggregator
app = Aggregator.new(true, ARGV)

has no advantage to simply writing

app = Aggregator.new(true, ARGV)

Using static members (@@stop) is also not advisable. If you are writing inside the self scope (inside def self.shutdown) simple members (@stop) are actually in the class scope, being effectively static. To access them from outside this scope, add a getter:

def self.stop
  @stop
end

Which will be accessible by calling Aggregator.stop.

Refactored Aggregator:

class Aggregator
  include Logging
  include Setting

  AGGREGATOR_VERSION = '0.0.1'

  def self.version
    AGGREGATOR_VERSION
  end

  attr_reader :environment, :plugin_manager, :stop

  def initialize(internal_server = false, arguments = [])
    options = ArgumentParser.parse(arguments)

    Logging::environment options.environment
    Logging::quiet = true if options.quiet

    logger.info 'Starting up aggregator now'

    logger.info "Using #{options.environment} environment"
    @environment = options.environment

    DatabaseInitializer.new(options.environment)

    return if options.environment == :test

    if options.console
      ConsoleInitializer.new(options.environment)
    else
      @plugin_manager = PluginManager.new 

      ServerInitializer.new if internal_server

      logger.info "Aggregator is up and running"

      Signal.trap("SIGINT") do
        Thread.current.exit
      end

      start
    end
  end

  def shutdown
    @stop = true
  end

  private

  def start
    until stop
      @plugin_manager.run

      logger.debug "Aggregation done. Next aggregation in #{setting.aggregate_timer} seconds."
      sleep setting.aggregate_timer
    end
  end
end
\$\endgroup\$
  • \$\begingroup\$ Thank you for this great review. I've no clue, why I can't replace my static members with getters and setters. When I access them via a static getter method they are always nil. github.com/openscript/social_aggregator/blob/master/… \$\endgroup\$ – Robin Feb 19 '14 at 18:07
  • \$\begingroup\$ What I still wonder, if it's a good idea to have this built in Webrick server beside my config.ru. What do you think about it? \$\endgroup\$ – Robin Feb 19 '14 at 18:08
  • \$\begingroup\$ @Sam - read my answer carefully about using static members - you should not use the @@ directive anywhere - to assign to the class variables call the setter (Aggregator.stop='my_version'), and it should work. Getters and setters in the class scope (def self.stop=) will set @stop to the class, not the instance, and vice versa. \$\endgroup\$ – Uri Agassi Feb 19 '14 at 18:34
  • \$\begingroup\$ I would not recommend using Webrick for anything but the development phase. Choose your webserver according to your application's sizing requirements. The simplest webserver I would advise you to look at is thin. See also stackoverflow.com/questions/10859671/… \$\endgroup\$ – Uri Agassi Feb 19 '14 at 18:37

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.