# First BDD/RSpec tests, would like some review regarding idioms, conventions and style

This is my first attempt at BDD and RSpec. I'm more of a C# xUnit kind of guy.

I wrote this class while developing a 2D random tile map generator (just for fun). The project is still ongoing.

I would particularly like some review on:

• Test granularity/scope in relation to BDD
• Use of RSpec
• Are these too much like unit tests?
• Adherence to Ruby and RSpec idioms, conventions, coding style

I am also open to any other kind of comments and review.

require './map_factory'

describe MapFactory, "map creation" do
before(:each) do
@map_factory = MapFactory.new
end

it "should return a new Map instance" do
map = @map_factory.make(20, 20)
map.should be_an_instance_of(Map)
end

it "should be a map of the specified size" do
map = @map_factory.make(20, 20)
map.width.should equal(20)
map.height.should equal(20)
map.tiles.length.should equal(400)
end

it "should be an island" do
map = @map_factory.make(20, 20)

# an island should have water all around it
for x in 0...map.width
map.tile_at(x, 0).type.should eq(:water)
map.tile_at(x, 19).type.should eq(:water)
end

for y in 0...map.height
map.tile_at(0, y).type.should eq(:water)
map.tile_at(19, y).type.should eq(:water)
end
end

it "should have more than half the tiles be landmass" do
map = @map_factory.make(40, 40)

number_of_water_tiles = 0
number_of_land_tiles = 0

map.tiles.each do |tile|
if tile.type == :water then
number_of_water_tiles += 1
else
number_of_land_tiles += 1
end
end

number_of_land_tiles.should be > number_of_water_tiles
end
end


I am no RSpec pro yet, but here are a few things I would have done differently (see the comments):

require './map_factory'

describe MapFactory, "map creation" do
before(:each) do
@map_factory = MapFactory.new
end

# I would put those 3 tests in the same context to DRY up things
context "20 x 20 map" do
before(:each) do
map_factory = MapFactory.new # EDIT: this was an instance variable, which is useless here
@map = map_factory.make(20, 20) # all three following tests used that, so why not use an instance variable here?
end

it "should return a new Map instance" do
@map.should be_an_instance_of(Map)
end

it "should be a map of the specified size" do
@map.width.should equal(20)
@map.height.should equal(20)
@map.tiles.length.should equal(400)
end

# you had a comment here, maybe it could be in the test description?
it "should be an island and have water all around it" do
# Enforce the use of Ruby ranges and iterator
(0...@map.width).each do |x|
@map.tile_at(x, 0).type.should eq(:water)
@map.tile_at(x, 19).type.should eq(:water)
end

(0...@map.height).each do |y|
@map.tile_at(0, y).type.should eq(:water)
@map.tile_at(19, y).type.should eq(:water)
end
end
end

it "should have more than half the tiles be landmass" do
number_of_water_tiles = 0
number_of_land_tiles = 0

@map.tiles.each do |tile|
if tile.type == :water # remove "then" here as it is not needed
number_of_water_tiles += 1
else
number_of_land_tiles += 1
end
end

number_of_land_tiles.should be > number_of_water_tiles
end
end


• I often see the advice to use each rather than a for loop. Is there any reason beyond stylistic reasons? – Gilles Jun 13 '12 at 21:05

Your tests are good. But they can be better)

• At first, use one should for one example( it ). Your test falls when first matcher in order return false. And conditions below won't be checked. It's good for finding bugs and refactoring. Green test is not the reason of thinking that your code haven't got any bugs :) Read this

• Don’t begin tests(examples) names with the word ‘should’ for readability. Results should be easy to read.

• Some developers very like custom matchers for readable and clean code. It's very cool, but it is not essential often

A small hint: the count method could clean up one of your test case, more or less as such:

it "should have more than half the tiles be landmass" do
map = @map_factory.make(40, 40)
number_of_water_tiles = map.tiles.count{|tile| tile.type == :water}
number_of_land_tiles = map.tiles.size - number_of_water_tiles
number_of_land_tiles.should be > (map.tiles.size / 2)
end
end


Also your map could go in the before section too imho.