Skip to main content
more issues
Source Link
Rene Saarsoo
  • 2.1k
  • 11
  • 14

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 like get(x,y) that would encapsulate the access to the coordinates array.
  • Screen has an inspect method that's unused. For debugging?
  • Screen has x and y fields, I think these are better called width and height.
  • Screen has a print method. I would instead recommend having a to_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 call puts screen to print it to console (the to_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.

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.

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 like get(x,y) that would encapsulate the access to the coordinates array.
  • Screen has an inspect method that's unused. For debugging?
  • Screen has x and y fields, I think these are better called width and height.
  • Screen has a print method. I would instead recommend having a to_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 call puts screen to print it to console (the to_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.
Source Link
Rene Saarsoo
  • 2.1k
  • 11
  • 14

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.