# Machi Koro card/dice game

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

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


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

• This is just what I was hoping for... a mixture of things I didn't know about (alias_method), things I did but forgot to do (symbols for hash keys) and dodgy design decisions (@attribute... in my head Establishment was just a glorified array so that's where my design process started). Can you expand on - or provide links to - why you would refactor the initialize method like that? I did think of your suggestion, but decided my way was clearer. – WelshGandalf Dec 11 '16 at 18:15
• @WelshGandalf initialize method this way is more concise, and it makes immediately obvious which attributes need casting to integer, and which need casting to symbol, because they are grouped together. If you do 8 separate assignments you need to read whole thing to be sure everything is right. – Borsunho Dec 11 '16 at 18:35
• OK I understand the POV better now. Couple more things. The @ attribute means I only have to do attr_reader for @attribute rather than for all the elements in it. If I make each element an instance variable instead, is there a shorthand rather than listing them all as attr_reader? Also, is there an alternative form of the .each block that would work with this approach, I couldn't figure one out myself. – WelshGandalf Dec 15 '16 at 0:41