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 Answer 1

Reset to default
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\$
4
  • \$\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\$ Commented Feb 19, 2014 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\$ Commented Feb 19, 2014 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\$ Commented Feb 19, 2014 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\$ Commented Feb 19, 2014 at 18:37

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.