I wrote a Langton's Ant cellular automaton simulator in Ruby using object oriented principles. This code works. It requires the drawille
library.
I would like to get feedback on what I could do better here, from optimizing the display to cleaning up code. I'm also wondering what I could improve in code quality and style.
#!/usr/bin/env ruby
require 'drawille'
# Langton's Ant
# Draws in terminal
class Direction
@dir = 0
def initialize(direction)
@dir = {up: 0, right: 1, down: 2, left: 3}[direction] || 0;
end
def transform_vector
case @dir
when 0
return {x: 0, y: 1}
when 1
return {x: 1, y: 0}
when 2
return {x: 0, y: -1}
when 3
return {x: -1, y: 0}
end
end
def turn_cw
@dir = (@dir + 1) % 4
end
def turn_ccw
@dir = (@dir + 3) % 4
end
end
module State
OFF = 0
ON = 1
end
class Tile
@state = State::OFF
def initialize(state)
@state = state
end
def state
return @state
end
def flip
if @state == State::OFF
return @state = State::ON
else
return @state = State::OFF
end
end
def bool
return (@state == State::ON ? true : false)
end
end
class Screen
@screen = [];
@canvas = Drawille::Canvas.new
@x = 0
@y = 0
def initialize(x, y)
@screen = Array.new(y) { Array.new(x) { Tile.new State::OFF } }
@canvas = Drawille::Canvas.new
@x = x
@y = y
end
def inspect
string = "";
@screen.each do |i|
i.each do |j|
string << (j.bool ? '██' : ' ')
end
string << ?\n
end
return string
end
def screen
return @screen
end
def flip(x, y)
return @screen[y][x].flip
end
def print
@screen.each_with_index do |row, y|
row.each_with_index do |cell, x|
if cell.state == State::ON
@canvas.set(x, y)
else
@canvas.unset(x, y)
end
end
end
puts @canvas.frame
end
def x
@x
end
def y
@y
end
end
class Ant
@x = 0
@y = 0
@direction = :up
@screen = nil
def initialize(x, y, direction, screen)
@x, @y, @direction, @screen = x, y, direction, screen
end
def current_cell
@screen.screen[@y][@x]
end
def step
if current_cell.state == State::ON
@direction.turn_cw
else
@direction.turn_ccw
end
@screen.flip(@x, @y)
@x = (@x + @direction.transform_vector[:x]) % @screen.x
@y = (@y + @direction.transform_vector[:y]) % @screen.y
end
end
def main
screen = Screen.new(160, 96);
ant = Ant.new(79, 47, Direction.new(:down), screen);
loop do
screen.print
ant.step
end
end
main
1 Answer 1
The code looks pretty clean. On first sight I notice some odd Ruby code.
Non-idiomatic Ruby
class Tile
@state = State::OFF
def initialize(state)
@state = state
end
def state
return @state
end
def flip
if @state == State::OFF
return @state = State::ON
else
return @state = State::OFF
end
end
def bool
return (@state == State::ON ? true : false)
end
end
In the code above you first set @state
and then inside initialize
you set @state
again. These two are not the same variable. Do not define instance variables outside class methods, unless you want to do some meta-programming with them.
You define method state
for accessing the @state
variable. As this is such a common pattern, Ruby provides a helper for generating such simpler getter methods:
class Tile
attr_accessor :state
But all the places where state
method is used, could actually use the bool
method instead. This should make one realize that this whole Tile class could be completely eliminated and replaced with a plain boolean value instead.
Other smells
- Screen class has
@screen
instance variable. It's not referring to itself, so a better name would be in order. - It's also exposing it directly to the outside world through
screen
method. Better to have a method likeget(x,y)
that would encapsulate the access to the coordinates array. - Screen has an
inspect
method that's unused. For debugging? - Screen has
x
andy
fields, I think these are better calledwidth
andheight
. - Screen has a
print
method. I would instead recommend having ato_s
method instead, so the Screen would not have a knowledge about printing itself, only of how to convert itself into string. One can then simply callputs screen
to print it to console (theto_s
method will be call automatically when casting to string). - Inside
Ant.initialize
all the variables are initialize on the same line, which I find confusing.
-
\$\begingroup\$ I have the
Tile
class for future extension (in case I want to implement some Langton's Ant variants). Thebool
method is one that I forgot to remove. I usedScreen#inspect
for output before I switched todrawille
. Thanks for the tips onattr_accessor
, field names, instance variables,to_s
, etc. I find the one line initialization cleaner and easier to read, but if it's confusing for most people I'll change it. \$\endgroup\$anna328p– anna328p2017年12月09日 01:00:29 +00:00Commented Dec 9, 2017 at 1:00
Explore related questions
See similar questions with these tags.
Tile
class and its collaborators go to a lot of effort to make0
and1
act like booleans. Why not just usefalse
andtrue
? \$\endgroup\$