Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

S2C::Config allow ad-hoc config_file_path and completely S2C::Config …

…decoupling
  • Loading branch information...
commit 5459c39c807c024278f7a20dad136934c13bafce 1 parent ee098f8
@fguillen authored
View
1  Gemfile
@@ -9,4 +9,5 @@ end
group :test do
gem 'mocha'
gem 'ruby-debug19'
+ gem 'delorean'
end
View
4 Gemfile.lock
@@ -2,7 +2,10 @@ GEM
remote: http://rubygems.org/
specs:
archive-tar-minitar (0.5.2)
+ chronic (0.3.0)
columnize (0.3.2)
+ delorean (1.0.0)
+ chronic
highline (1.6.2)
linecache19 (0.5.12)
ruby_core_source (>= 0.1.4)
@@ -23,6 +26,7 @@ PLATFORMS
ruby
DEPENDENCIES
+ delorean
highline
mocha
rack
View
18 lib/s2c/config.rb
@@ -2,21 +2,21 @@
module S2C
class Config
-
+
CONFIG_PATH = "#{File.dirname(__FILE__)}/../../config/config.yml"
-
- def self.config_path
- S2C::Config::CONFIG_PATH
- end
- def self.[](key)
- config[key]
+ def initialize( config_path = CONFIG_PATH )
+ load( config_path )
+ end
+
+ def []( key )
+ @config[key]
end
private
- def self.config
- @config ||= YAML.load( File.read( S2C::Config.config_path ) )
+ def load( config_path )
+ @config ||= YAML.load( File.read( config_path ) )

why not do this directly in #initialize?

@fguillen Owner

Think this is better for testing, maintenance and for incentivize atomicness.

I guess I'm confused, #load is a private method, and is only called by #initialize. You shouldn't be testing private methods directly. Could you give me an example where this helps testing, maintenance, or incentivizes atomicness? This looks to me like a one-line helper method. Moving it into #initialize would actually speed up your code base by not making an additional method call when instantiating a new Config object.

@fguillen Owner

The confused can be me.. just explaining my point of view:

maintenance

Today is only one line.. tomorrow could be a completely another thing, but the load() call wil be the same.

atomicness

I want to have a method where the initializations are done but without the another implicit things that an initialize (constructor) method does.

testing

I don't know why I shouldn't be testing private methods. Even if they are not going to be part of the public API they have to be tested, maybe in development to see if they are doing what they are expected to.

But also for testing proposes is very helpful to be able to mock this method to build testing instances of the object without executing the whole load chain.. again now is a line but tomorrow could be a complet chain of sentences, even expensive ones.

Thanks for elaborating. These all sound good, though I want to mention a couple things. In regards to maintenance, make sure you don't prematurely optimize your code. I usually find that the assumptions I make up front in a project a wrong and it's better to work with as few assumptions at first as possible. I think your atomicness argument is the best reason to keep this code out of #initialize. Finally, make sure you never directly test private methods. Since users of your library can't access your private methods, you shouldn't test them that way yourself. Instead, make sure you are thoroughly testing the methods that call your private methods. If you feel it necessary to test a private method, then I've often found that the method might need to be public.

@fguillen Owner

I completely agree with the no premature optimization, and I understand what you say with no testing private methods.. maybe the problem is that it shouldn't have been a private method since the very beginning .. thx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
end
View
5 lib/s2c/console.rb
@@ -2,8 +2,11 @@
module S2C
class Console
+ attr_reader :config
+
def initialize
- @universe = Universe.new( S2C::Config['universe'] )
+ @config = S2C::Config.new
+ @universe = Universe.new( config )
@universe.start
@exit = false
end
View
4 lib/s2c/models/construction.rb
@@ -37,7 +37,7 @@ def upgrade_timing
end
def property_value( property )
- init_value = S2C::Config[type][property]
+ init_value = universe.config[type][property]
init_value + ( init_value * ( @level * ( 1.1 ** @level ) ) ).round
end
@@ -97,7 +97,7 @@ def stats
if status != :standby
result += "remaining_ticks:#{process_remaining_ticks}".ljust( 20 )
- result += "ending_time:#{S2C::Utils.remaining_ticks_to_time( process_remaining_ticks ).strftime( '%Y-%m-%d %H:%M:%S' )}"
+ result += "ending_time:#{S2C::Utils.remaining_ticks_to_time( process_remaining_ticks, universe.config['universe']['tick_seconds'] ).strftime( '%Y-%m-%d %H:%M:%S' )}"
end
result
View
4 lib/s2c/models/ship.rb
@@ -25,7 +25,7 @@ def travel( planet_destiny )
return false
end
- needed_black_stuff = S2C::Utils.travel_black_stuff( planet, planet_destiny )
+ needed_black_stuff = S2C::Utils.travel_consume_black_stuff( planet, planet_destiny, universe.config['universe']['travel_black_stuff'] )
if( planet.black_stuff < needed_black_stuff )
universe.log( self, "ERROR: not enough black stuff" )
@@ -65,7 +65,7 @@ def stats
if status != :standby
result += "remaining_ticks:#{process_remaining_ticks}".ljust( 20 )
- result += "ending_time:#{S2C::Utils.remaining_ticks_to_time( process_remaining_ticks ).strftime( '%Y-%m-%d %H:%M:%S' )}"
+ result += "ending_time:#{S2C::Utils.remaining_ticks_to_time( process_remaining_ticks, universe.config['universe']['tick_seconds'] ).strftime( '%Y-%m-%d %H:%M:%S' )}"
end
result
View
9 lib/s2c/universe.rb
@@ -2,12 +2,13 @@
module S2C
class Universe
- attr_accessor :planets, :logs, :status, :tick, :size
+ attr_accessor :planets, :logs, :status, :tick, :size, :config
- def initialize( opts )
+ def initialize( config )
@logs = []
@planets = []
- @size = opts['size']
+ @config = config
+ @size = config['universe']['size']
@tick = 0 # Universe's time
end
@@ -50,7 +51,7 @@ def run
self.cycle
end
- sleep( S2C::Config['universe']['tick_seconds'].to_i - time )
+ sleep( config['universe']['tick_seconds'].to_i - time )
end
self.log( self, "End run" )
View
10 lib/s2c/utils.rb
@@ -1,7 +1,7 @@
module S2C
- class Utils
- def self.remaining_ticks_to_time( remaining_ticks )
- seconds = remaining_ticks * S2C::Config['universe']['tick_seconds']
+ module Utils
+ def self.remaining_ticks_to_time( remaining_ticks, tick_seconds )
+ seconds = remaining_ticks * tick_seconds
Time.now + seconds
end
@@ -9,9 +9,9 @@ def self.planet_distance( planet1, planet2 )
Math.sqrt( ( planet2.position[0] - planet1.position[0] ) ** 2 + ( planet2.position[1] - planet1.position[1] ) ** 2 ).round
end
- def self.travel_black_stuff( planet1, planet2 )
+ def self.travel_consume_black_stuff( planet1, planet2, travel_black_stuff )
distance = S2C::Utils.planet_distance( planet1, planet2 )
- distance * S2C::Config['universe']['travel_black_stuff']
+ distance * travel_black_stuff
end
def self.travel_ticks( planet1, planet2, velocity )
View
8 test/config_test.rb
@@ -2,10 +2,10 @@
class ConfigTest < Test::Unit::TestCase
def test_config
- S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
+ config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
- assert_equal( 1, S2C::Config['universe']['tick_seconds'] )
- assert_equal( 11, S2C::Config['mine']['defense'] )
- assert_equal( 30, S2C::Config['ship']['attack'] )
+ assert_equal( 1, config['universe']['tick_seconds'] )
+ assert_equal( 11, config['mine']['defense'] )
+ assert_equal( 30, config['ship']['attack'] )
end
end
View
7 test/construction_test.rb
@@ -2,10 +2,9 @@
class ConstructionTest < Test::Unit::TestCase
def setup
- S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
-
- @universe = S2C::Universe.new( { 'size' => 20 } )
- @planet = @universe.create_planet( 'jupiter' )
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
+ @universe = S2C::Universe.new( @config )
+ @planet = @universe.create_planet( 'jupiter' )
end
def test_initialize
View
7 test/mine_test.rb
@@ -2,10 +2,9 @@
class MineTest < Test::Unit::TestCase
def setup
- S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
-
- @universe = S2C::Universe.new( { 'size' => 20 } )
- @planet = @universe.create_planet( 'jupiter' )
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
+ @universe = S2C::Universe.new( @config )
+ @planet = @universe.create_planet( 'jupiter' )
end
def test_initialize
View
6 test/planet_test.rb
@@ -2,10 +2,10 @@
class PlanetTest < Test::Unit::TestCase
def setup
- S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
- @universe = S2C::Universe.new( { 'size' => 20 } )
- @planet = @universe.create_planet( 'jupiter' )
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
+ @universe = S2C::Universe.new( @config )
+ @planet = @universe.create_planet( 'jupiter' )
end
def test_initialize
View
11 test/ship_test.rb
@@ -3,9 +3,8 @@
class ShipTest < Test::Unit::TestCase
def setup
- S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
-
- @universe = S2C::Universe.new( { 'size' => 20 } )
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
+ @universe = S2C::Universe.new( @config )
@planet = @universe.create_planet( 'jupiter' )
end
@@ -32,7 +31,7 @@ def test_travel
ship.planet.instance_variable_set( :@black_stuff, 12 )
- S2C::Utils.expects( :travel_black_stuff ).with( ship.planet, planet_destiny ).returns( 11 )
+ S2C::Utils.expects( :travel_consume_black_stuff ).with( ship.planet, planet_destiny, 2 ).returns( 11 )
S2C::Utils.expects( :travel_ticks ).with( ship.planet, planet_destiny, ship.velocity ).returns( 3 )
ship.travel( planet_destiny )
@@ -66,7 +65,7 @@ def test_travel_not_enough_black_stuff_should_returns_false
ship.planet.expects( :black_stuff ).returns( 10 )
- S2C::Utils.expects( :travel_black_stuff).with( ship.planet, planet_destiny ).returns( 11 )
+ S2C::Utils.expects( :travel_consume_black_stuff).with( ship.planet, planet_destiny, 2 ).returns( 11 )
assert_equal( false, ship.travel( planet_destiny ) )
assert_equal( :standby, ship.status )
@@ -102,7 +101,7 @@ def test_work_traveling_finished
end
def test_stats_in_traveling
- S2C::Utils.stubs( :remaining_ticks_to_time ).returns( Time.new( 2010, 11, 12, 13, 14, 15 ) )
+ S2C::Utils.expects( :remaining_ticks_to_time ).with( 14, @config['universe']['tick_seconds'] ).returns( Time.new( 2010, 11, 12, 13, 14, 15 ) )
planet_destiny = @universe.create_planet( 'x700' )
View
4 test/stats_test.rb
@@ -2,11 +2,11 @@
class StatsTest < Test::Unit::TestCase
def setup
- S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
end
def test_stats
- universe = S2C::Universe.new( { 'size' => 20 } )
+ universe = S2C::Universe.new( @config )
planet1 = universe.create_planet( 'x100', [1,2] )
planet2 = universe.create_planet( 'x200', [5,8] )
View
1  test/test_helper.rb
@@ -2,5 +2,6 @@
require 'test/unit'
require 'mocha'
require 'ruby-debug'
+require 'delorean'
FIXTURES_PATH = File.expand_path( "#{File.dirname(__FILE__)}/fixtures" )
View
14 test/universe_test.rb
@@ -1,17 +1,21 @@
require_relative 'test_helper'
class UniverseTest < Test::Unit::TestCase
+ def setup
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
+ end
+
def test_initialize
- universe = S2C::Universe.new( { 'size' => 20 } )
+ universe = S2C::Universe.new( @config )
assert_equal( [], universe.logs )
assert_equal( [], universe.planets )
assert_equal( 0, universe.tick )
- assert_equal( 20, universe.size )
+ assert_equal( 10, universe.size )
end
def test_create_planet
- universe = S2C::Universe.new( { 'size' => 20 } )
+ universe = S2C::Universe.new( @config )
planet = universe.create_planet( 'jupiter', [1,2] )
@@ -21,7 +25,7 @@ def test_create_planet
end
def test_cycle
- universe = S2C::Universe.new( { 'size' => 20 } )
+ universe = S2C::Universe.new( @config )
universe.instance_variable_set( :@tick, 1 )
planet = universe.create_planet( 'jupiter' )
mine = planet.build_mine
@@ -37,7 +41,7 @@ def test_cycle
def test_ships
- universe = S2C::Universe.new( { 'size' => 20 } )
+ universe = S2C::Universe.new( @config )
planet1 = universe.create_planet( 'jupiter' )
mine1 = planet1.build_mine
View
18 test/utils_test.rb
@@ -2,11 +2,12 @@
class UtilsTest < Test::Unit::TestCase
def setup
+ @config = S2C::Config.new( "#{FIXTURES_PATH}/config.yml" )
S2C::Config.stubs( :config_path ).returns( "#{FIXTURES_PATH}/config.yml" )
end
def test_planet_distance
- universe = S2C::Universe.new( { 'size' => 20 } )
+ universe = S2C::Universe.new( @config )
planet1 = S2C::Models::Planet.new( universe, 'jupiter', [0,0] )
planet2 = S2C::Models::Planet.new( universe, 'saturn', [0,0] )
@@ -31,10 +32,10 @@ def test_planet_distance
def test_travel_black_stuff
S2C::Utils.expects( :planet_distance ).returns( 1 )
- assert_equal( 2, S2C::Utils.travel_black_stuff( nil, nil ) )
+ assert_equal( 2, S2C::Utils.travel_consume_black_stuff( nil, nil, 2 ) )
S2C::Utils.expects( :planet_distance ).returns( 2 )
- assert_equal( 4, S2C::Utils.travel_black_stuff( nil, nil ) )
+ assert_equal( 4, S2C::Utils.travel_consume_black_stuff( nil, nil, 2 ) )
end
def test_travel_ticks
@@ -44,4 +45,15 @@ def test_travel_ticks
S2C::Utils.expects( :planet_distance ).returns( 8 )
assert_equal( 4, S2C::Utils.travel_ticks( nil, nil, 2 ) )
end
+
+ def test_remaining_ticks_to_time
+ Delorean.time_travel_to( Time.new( 2010, 11, 12, 13, 14, 15 ) ) do
+ finish_time = S2C::Utils.remaining_ticks_to_time( 2, 1800 )
+
+ assert_equal(
+ "2010-11-12 14:14:15",
+ finish_time.strftime( '%Y-%m-%d %H:%M:%S' )
+ )
+ end
+ end
end
View
2  wiki/TODO.md
@@ -1,4 +1,4 @@
# TODO
* planet names modify the map
-* S2C::Config.universe.size
+* S2C::Config.universe.size not S2C::Config['universe']['size']
@semmons99

why not do this directly in #initialize?

@fguillen

Think this is better for testing, maintenance and for incentivize atomicness.

@semmons99

I guess I'm confused, #load is a private method, and is only called by #initialize. You shouldn't be testing private methods directly. Could you give me an example where this helps testing, maintenance, or incentivizes atomicness? This looks to me like a one-line helper method. Moving it into #initialize would actually speed up your code base by not making an additional method call when instantiating a new Config object.

@fguillen

The confused can be me.. just explaining my point of view:

maintenance

Today is only one line.. tomorrow could be a completely another thing, but the load() call wil be the same.

atomicness

I want to have a method where the initializations are done but without the another implicit things that an initialize (constructor) method does.

testing

I don't know why I shouldn't be testing private methods. Even if they are not going to be part of the public API they have to be tested, maybe in development to see if they are doing what they are expected to.

But also for testing proposes is very helpful to be able to mock this method to build testing instances of the object without executing the whole load chain.. again now is a line but tomorrow could be a complet chain of sentences, even expensive ones.

@semmons99

Thanks for elaborating. These all sound good, though I want to mention a couple things. In regards to maintenance, make sure you don't prematurely optimize your code. I usually find that the assumptions I make up front in a project a wrong and it's better to work with as few assumptions at first as possible. I think your atomicness argument is the best reason to keep this code out of #initialize. Finally, make sure you never directly test private methods. Since users of your library can't access your private methods, you shouldn't test them that way yourself. Instead, make sure you are thoroughly testing the methods that call your private methods. If you feel it necessary to test a private method, then I've often found that the method might need to be public.

@fguillen

I completely agree with the no premature optimization, and I understand what you say with no testing private methods.. maybe the problem is that it shouldn't have been a private method since the very beginning .. thx.

Please sign in to comment.
Something went wrong with that request. Please try again.