Machi Koro card/dice game

Question

I have taken advice on board and had a go at a new project, to write a program that lets you play the card/dice game, Machi Koro. The complexity of the game isn't too great, which makes it a good target.

What I would like reviewing is the first "real-world" class I have created, the Establishment. There are many different establishments but there are many attributes they all have - a name, a cost, a colour and so on. I would like to be able to play the game via the console - as well as be written in such a way that another front end could interface with fairly easily. Here is the output of running console_output on one of them (it appears in blue). It might help you understand what the code is trying to model.

**************************************
*                 1                  * Get 1 coin from the bank,
*            Wheat field             * on anyone's turn.
*           Symbol : grain           *
*              Cost : 1              *
**************************************

I have been trying to develop using TDD and RSpec. However when it came to console_output I thought the best thing was for me to view the output myself... after all I knew exactly how I wanted it to look, and I don't think I could programmatically write something in RSpec to ensure it showed up in the colour I was expecting. Is this an OK approach? I have written no tests at all for console_output.

The class is not complete yet, but everything that's there will definitely be used later on. I don't think there's much more I can do until I flesh out how the Game class is going to work exactly. Is that good practice? I'm no expert in OOP.

The Databank class will eventually include other things - other types of cards, rules definitions. The idea is, it takes data from the database (via the db_access file) and creates nice Ruby objects for all of them.

I ran the code through some code quality checks and got the following things back.:

• initialize has too many assignments - but it seems the clearest way of doing things to me. I have, however, been told by someone else that I tend to put too much in my initialization calls.
• console_output has too many assignments - but it seems the clearest way of doing things to me (1 LOC = 1 output line)
• is_activated's cyclomatic complexity is too high - perhaps it could be rewritten with if/then but it seems fine to me

I've also tried to follow the Ruby style guide. I've broken it a couple of times that I can see... some lines are over 80 characters (maybe 1 of them really is too long), and I've used double-assignment once.

There are some other things I have written which don't seem too great to me... but I'm unsure how to do them in a better way. I feel this way a lot at this stage of my development - that I am getting better at identifying code smells, but not at fixing them. I was recently assessed as having beginner to intermediate Ruby skills - help me level up to intermediate!

I've replaced some code to do with colouring the text with "it works" type comments. That code isn't important, it's not what I would like reviewed and it works just fine.

require_relative 'db_access'
require 'json'
class Colours
   # some ANSI codes
end

class String
   def colour(colour) #this works
   end
end


module MachiKoro

  class Establishment

    attr_reader :attribute, :id

    def initialize(data)
      @attribute = Hash.new()
      @id = data["id"].to_i
      @attribute["name"] = data["description"]
      @attribute["effect"] = data["effect"]
      @attribute["cost"] = data["cost"].to_i
      @attribute["from_roll"] = data["from_roll"].to_i
      @attribute["to_roll"] = data["to_roll"].to_i
      @attribute["base_income"] = data["base_income_value"].to_i

      #TODO id's that will activate the building

      @attribute["colour"] = data["colour"].downcase.to_sym
      @attribute["symbol"] = data["symbol"].downcase.to_sym #haha
      @attribute["expansion"] = data["expansion"].downcase.to_sym
      @attribute["alternative_income_method"] = data["alternative_income_method"].to_sym if !data["alternative_income_method"].nil? # line too long?
    end

    def to_json
      @attribute.to_json
    end

    def is_activated(roll, owners_turn)
      return false if !(@attribute["from_roll"] <= roll && @attribute["to_roll"] >= roll)
      return true if @attribute["colour"] == :blue
      return true if @attribute["colour"] == :green && owners_turn
      return true if @attribute["colour"] == :red && !owners_turn
      false # a catch-all... shouldn't really happen
    end

    def justified_effect
      return @justified_effect if defined? @justified_effect
      @justified_effect = Array.new
      strlen, i = 0, 0
      @attribute['effect'].split.each do |word|
        if strlen > 25 #can these 4 lines be shortened to one?
          i += 1
          strlen = 0
        end
        @justified_effect[i] = '' if @justified_effect[i].nil?
        @justified_effect[i] << word + ' '
        strlen += word.length + 1
      end
      5.times { @justified_effect << '' } #ensures no Nil elements
      @justified_effect.collect { |line| line.chop }
    end

    def je; self.justified_effect; end

    def roll_range
      #this COULD be done on one line... but it would have poor readability
      if @attribute["from_roll"] == @attribute["to_roll"]
        return @attribute["from_roll"].to_s
      end
      "#{@attribute["from_roll"]}-#{@attribute["to_roll"]}"
    end

    def console_output
      w = 36 # width of "card"
      str = '*' * (w+2) << "\n"
      str << '*' << self.roll_range.center(w) << "* #{je[0]}\n"
      str << '*' << @attribute["name"].capitalize.center(w) << "* #{je[1]}\n"
      str << '*' << " Symbol : #{@attribute["symbol"]} ".center(w) << "* #{je[2]}\n"
      str << '*' << " Cost : #{@attribute["cost"]} ".center(w) << "* #{je[3]}\n"
      str << '*' * (w+2) << " #{je[4]}"
      puts str.colour(ansi_colour)
    end

    private
    def ansi_colour
      case @attribute["colour"]
        when :red
          Colours::RED
        #and so on for other colours
      end
    end
  end

  class Databank

    def initialize
      @db = DBAccess.new
    end

    def establishments
      return @establishments if defined? @establishments
      @establishments = Array.new()
      @db.get_all_establishments.each do |data|
        @establishments << MachiKoro::Establishment.new(data)
      end
      @establishments
    end

  end

end

Answer 1

def je; self.justified_effect; end

Use alias_method for this.

@attribute["name"] = data["description"]

Unless you are using frozen string literals, you should use symbols for hash keys. They have superior performance. If you do use frozen string literals, you should use symbols for hash keys as well. Everybody reading your code will expect them, and they are better supported by syntax.

[:cost, :from_roll, :to_roll, :base_income_value].each do |attr|
  @attribute[attr] = data[attr].to_i
end

You would need another such loop for :to_sym, but I firmly believe it is worth it.

def console_output
  w = 36 # width of "card"
  ...
end

w here should be a class constant, so that it is easily accessible. Even if no other part of your code needs to know width of card, you yourself will have easier time finding it if it needs fine tuning.

You have @attribute["xxx"] repeated all over your code, which makes it clunky. However, the only reason that would justify existence of this hash over storing attributes in instance variables seems to be to_json method. It would benefit the code as a whole to use ordinary attr_accessor, and instead declare what goes into json in to_json method, even if you would just type that by hand:

def to_json
  {
    effect: effect,
    #...
  }.to_json
end