HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyModerate

Creating a 2D array of map tiles

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
creatingtilesmaparray

Problem

I am a C++ programmer new to Ruby and someone suggested that I post my code here since it doesn't quite match Ruby standards. Is there a more Ruby way of doing this?

I have created a 2D array full of MapTile class objects and then I draw the tiles to the screen by iterating through the 2D Array using a for loop. Here is some code.

class MapTile
attr_accessor :tileSprite, :attribute

    def initialize(sprite, attr)
    @tileSprite = sprite
    @attribute = attr
    end 

    def tileSprite
        @tileSprite
    end

    def attribute
        @attribute
    end

end

def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end

@mapData = array2D(@mapSize,@mapSize)

for i in 0..@mapSize - 1
   for j in 0..@mapSize - 1
      mapData[i][j].tileSprite = tileNum
      @tile.draw(mapData[i][j].tileSprite)
   end 
end

Solution

First of all your indentation is a bit wacky. Sometimes you indent by 4 spaces, sometimes by 2 (which is the default in the ruby community) and sometimes not at all (for example you didn't indent the call to attr_accessor or the contents of the initialize method). You should fix that.

attr_accessor :tileSprite, :attribute
# ...
def tileSprite
    @tileSprite
end

def attribute
    @attribute
end


No need to call attr_accessor and define the getter methods yourself. Do one or the other.

def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end


Creating an array containing a default value and then mapping over that array to replace the default value makes very little sense. Why assign a default value at all if you throw it away right after?

Also it's a very bad idea to make the inner arrays all contain a reference to the same map tile. This just screams for you to introduce hard to find bugs. As a guide line you should never use the 2-argument version of Array.new with a mutable value as its argument. Use the block-version instead.

Also it's customary in ruby to not use explicit return statements in the last line of a method.

Here's how I'd write the array2d method:

def array2D(width,height)
  Array.new(width) do
    Array.new(height) do
      MapTile.new(123,0)
    end
  end
end

Code Snippets

attr_accessor :tileSprite, :attribute
# ...
def tileSprite
    @tileSprite
end

def attribute
    @attribute
end
def array2D(width,height)
  a = Array.new(width, MapTile.new(123,0))
  a.map! { Array.new(height, MapTile.new(123,0)) }
  return a
end
def array2D(width,height)
  Array.new(width) do
    Array.new(height) do
      MapTile.new(123,0)
    end
  end
end

Context

StackExchange Code Review Q#10550, answer score: 12

Revisions (0)

No revisions yet.