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 - bu

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

StackExchange Code Review Q#149517, answer score: 2