Code review for Practicing Ruby 4.3 #5

Open
wants to merge 21 commits into
from

Conversation

Projects
None yet
2 participants
Owner

practicingruby commented Apr 28, 2012

Please feel free to leave your own comments and questions in the code.

- game.move( 0.2, 0.0) if holding?(:d)
-
- position = game.player_position
+game_runner = Blind::UI::GameRunner.new
@practicingruby

practicingruby Apr 28, 2012

Owner

Extracting the Ray code out into its own class was necessary for making it easier to do UI testing.

+require_relative "worlds"
+
+module Blind
+ module Games
@practicingruby

practicingruby Apr 28, 2012

Owner

This factory object is what I'm currently using to put together a sequence of worlds to be played in order, along with their descriptions. It would probably be much better as some sort of DSL, or even a JSON file.

+
+module Blind
+ module Worlds
+ def self.original(mine_count)
@practicingruby

practicingruby Apr 28, 2012

Owner

Similar to Blind::Games, this code could probably benefit from a nice DSL. But in the code below you will be able to see how my generalized world objects are meant to be used. The Blind::Worlds.original reflects the layout of the world used in Practicing Ruby 4.2

@@ -11,7 +11,7 @@ def initialize(world)
@practicingruby

practicingruby Apr 28, 2012

Owner

Notice how the World object is now fairly ignorant of game-specific terms. It is up to the Game object (and the config/worlds.rb file) to introduce those. This is a win, but does make the World object a bit more cumbersome to use.

@@ -0,0 +1,9 @@
@practicingruby

practicingruby Apr 28, 2012

Owner

This is about as simple of an object as you can imagine, but it's mostly because I didn't really add any abstraction at all. It probably can be the home of more interesting level specific logic later.

+ x = length*Math.cos(angle)
+ y = length*Math.sin(angle)
+
+ point = new(x, y)
@practicingruby

practicingruby Apr 28, 2012

Owner

If it weren't for floating point rounding errors, this point would be guaranteed to be within the distance_range from (0,0).

+ point = new(x, y)
+ center = new(0, 0)
+
+ if distance_range.include?(point.distance(center))
@practicingruby

practicingruby Apr 28, 2012

Owner

Because there is a possibility for floating point precision errors, I do rejection sampling and generate a new point if it is not in the distance range. This is messy business, and is possibly a sign of a bad model. The problem is that the borders between the regions in the game are zero-width, and so a rounding error that causes a point to be placed at (0,19.99999999997) instead of (0,20) could end up in a completely different region and cause problems. Rejecting points that aren't within the specified range at least guarantees that the error will be invisible to the player.

Unfortunately, there are other areas where point arithmetic can lead to rounding errors. They do not affect gameplay but do affect tests on occasion. This is one of several painful rats nests I ended up in when working on this codebase :-/

@@ -27,7 +46,7 @@ def to_a
end
def to_s
- "(#{x}, #{y})"
+ "(#{'%.2f' % x}, #{'%.2f' % y})"
@practicingruby

practicingruby Apr 28, 2012

Owner

Before the points we chose were truncated to integers, now that they aren't we need to prevent displaying giant unformatted floats. But I sort of hate doing formatting code in data structures, perhaps this belongs elsewhere?

def initialize(x,y)
@data = Vector[x,y]
end
+ attr_accessor :label
@practicingruby

practicingruby Apr 28, 2012

Owner

This is a clever tagging trick which facilitates easy lookup of points via the PointSet object that World wraps its points in. it's a bit of a kludge but works surprisingly well.

-
+ def initialize(levels)
+ # FIXME: stopgap measure, should be non-destruction
+ @levels = Marshal.load(Marshal.dump(levels))
@practicingruby

practicingruby Apr 28, 2012

Owner

Total hack. This is a sign I am trying too hard! I should instead be creating an Enumerator here, or storing and incrementing an index.

+
+ attr_accessor :message, :current_level
+
+ def load_new_level
@practicingruby

practicingruby Apr 28, 2012

Owner

This is definitely a sign of an object that needs to be extracted. Whenever you have a method that says "Let's delete all the variables and re-initialize them", you are essentially recreating object creation/destruction mechanisms. Still, I struggled to find the clear dividing lines here.

-
- sounds[:siren].volume =
- ((world.distance(world.center_position) - min) / max.to_f) * 100
+ sounds[:siren].volume = world.regional_depth * 100
@practicingruby

practicingruby Apr 28, 2012

Owner

I was really happy to get this logic out of the UI code, although World#regional_depth is an incredibly vague method name.

setup_sounds
setup_events
end
- attr_reader :game_over_message
@practicingruby

practicingruby Apr 28, 2012

Owner

Needed to remove this because the presenter needs to present messages throughout the game, not just at the end.

end
def finished?
- !!game_over_message
+ finished
@practicingruby

practicingruby Apr 28, 2012

Owner

Determining whether the game is finished now relies on a boolean field, which is actually a bit cleaner than checking for the presence of a display message.

@@ -0,0 +1,49 @@
@practicingruby

practicingruby Apr 28, 2012

Owner

While it was a bit tedious to set this up, it makes it possible for us to both run the game as an executable script, and also control it grammatically from Blind::UI::Simulator testing tool

@@ -0,0 +1,70 @@
@practicingruby

practicingruby Apr 28, 2012

Owner

This whole tool is a huge hack, and required tons of trial and error with mostly undocumented Ray functionality. However, it does seem to allow us to simulate a player pressing the WASD keys in an attempt to reach a certain position in the world, which allows for complete outside-in UI testing.

+
+ attr_reader :scene
+
+ def move(x,y)
@practicingruby

practicingruby Apr 28, 2012

Owner

This method is horribly long and ripe for refactoring. It is also a bit of a minefield: if you aren't careful about how you handle the conditions, it is easy to accidentally forget to press/release the right buttons at the right time, which can cause the acceptance tests to hang completely.

- def initialize(mine_count)
- @center_position = Blind::Point.new(0,0)
- @current_position = Blind::Point.new(0,0)
+ class PointSet
@practicingruby

practicingruby Apr 28, 2012

Owner

This makes searching through the Point objects that World aggregates easier. In retrospect, it would have been better to store these objects in a hash keyed by point label (with a catchall for unlabeled points, possibly). That would have been much more efficient and also look better in the code.

+ end
+
+ def first(label)
+ @points.find { |point| point.label == label }
@practicingruby

practicingruby Apr 28, 2012

Owner

If we had used a hash, this would look like

@points[label].first
+ end
+
+ def all(label)
+ @points.select { |point| point.label == label }
@practicingruby

practicingruby Apr 28, 2012

Owner

If we had used a hash, this would look like

@points[label]
- @mine_positions = mine_count.times.map do
- random_minefield_position
+ def <<(point)
+ @points << point
@practicingruby

practicingruby Apr 28, 2012

Owner

If we had used a hash, this would look like:

@points[point.label] << point
+
+ def region_at(point)
+ distance = point.distance(center_point)
+
@practicingruby

practicingruby Apr 28, 2012

Owner

This awkward code is a sign that some object similar to PointSet should exist for aggregating regions.

- x = length*Math.cos(angle)
- y = length*Math.sin(angle)
@practicingruby

practicingruby Apr 28, 2012

Owner

This awkward code is a sign that some object similar to PointSet should exist for aggregating regions.

-
- def random_minefield_position
- angle = rand(0..2*Math::PI)
- length = rand(MINE_FIELD_RANGE)
@practicingruby

practicingruby Apr 28, 2012

Owner

This awkward code is a sign that some object similar to PointSet should exist for aggregating regions.

@@ -0,0 +1,75 @@
@practicingruby

practicingruby Apr 28, 2012

Owner

Adding these acceptance tests was a mixed blessing. On the one hand, it is a very rigorous test because it verifies the ability to play through an entire game using the same inputs as a player would enter. On the other hand, it is extremely fragile code and even small problems can cause it to hang. This feels somewhat familiar to using something like Mechanize to test a Rails application: it gives you a very similar testing environment to the real world use case, but is very prone to false positives and false negatives, as well as general "OMG WHAT JUST HAPPENED!?!" moments.

describe Blind::Game do
- # NOTE: use just one mine to make testing easier
- let(:world) { Blind::World.new(1) }
+ let(:world) do
@practicingruby

practicingruby Apr 28, 2012

Owner

This is a somewhat evil kludge to deal with some floating point rounding errors which do not affect gameplay but DO affect test runs. Because I was having trouble isolating all potential sources of error and the floating point errors do not really affect gameplay, I decided to marshal a World object during one of the successful test runs. The problem with this of course is that it masks an underlying problem that may lead to unexpected results in other places.

I will be looking into suggestions on how to deal with this sort of problem later.

@@ -4,3 +4,7 @@
require_relative "point_test"
require_relative "world_test"
require_relative "game_test"
+
+if ENV['TEST_ALL']
@practicingruby

practicingruby Apr 28, 2012

Owner

Because the acceptance tests fire up a Ray UI, and because they aren't really needed to be run every time, I disable them by default and force the use of an environment variable to enable them.

-describe Blind::World do
+# FIXME: SPLIT OUT GENERAL TESTS FROM SCENARIO INDEPENDENT TESTS.
@practicingruby

practicingruby Apr 28, 2012

Owner

Through various stages of refactoring, World became less and less aware of Blind's game rules. However, these tests still sort of cover both generic functionality and game-specific rules, and need to be cleaned up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment