How do I compare two integer value hashes?

Question

I created a function that compares two hashes and returns a report on whether the subtraction of their values is negative or not.

The problem comes down to having a cost hash for a building and a current resources hash like:

cost = {:wood => 300, :stone => 200, :gold => 100}
reqs = {:wood => 200, :stone => 220, :gold => 90}

This one should return a report hash like:

report = { :has_resources => false, :has_wood => true, :has_stone => false, :has_gold => true } 

Now, in the cost hash, :gold, :stone or :wood can be nil, i.e. non-existent.

My first attempt is definitely not the Ruby way and I don't like the function. It works, but I want to find a way to write it in a better manner:

def has_resources?(cost)
 report = { :has_resources => true, :has_wood => true, :has_stone => true, :has_gold => true } 
 if not cost[:wood].nil? 
 if self.wood < cost[:wood]
 report[:has_wood] = false
 report[:has_resources] = false 
 end
 end
 if not cost[:stone].nil? 
 if self.stone < cost[:stone]
 report[:has_stone] = false
 report[:has_resources] = false
 end
 end 
 if not cost[:gold].nil?
 if self.gold < cost[:gold]
 report[:has_gold] = false
 report[:has_resources] = false
 end
 end
end

How should I rewrite this? I don't like the .nil? checks here, but I have to include them since the < operator does not work on nil objects. I also don't like having so many ifs.

Answer 1

Here's a version that is a bit more DRY. It also makes use of ruby's default value for hashes, and I threw in a dummy class in order to make the tests actually run.

This could probably be done even better, but one thing you really should try to achieve when writing ruby code (or any code at all, for that matter) is to try not to repeat yourself too much. That's where DRY come in.

class CostCalculation
 attr_accessor :wood, :stone, :gold
 def initialize(reqs)
 self.wood = reqs[:wood] || 0
 self.stone = reqs[:stone] || 0
 self.gold = reqs[:gold] || 0
 end 
 def has_resources?(cost)
 report = Hash.new(true)
 cost.default = 0 
 attributes.each do |key, value|
 report[:"has_#{key}"] = cost[key] <= value
 end 
 if report.values.include?(false)
 report[:has_resources] = false
 end 
 report
 end 
 def attributes
 { wood: self.wood, stone: self.stone, gold: self.gold } 
 end 
end

...and here are some specs for it as well:

describe CostCalculation do
 it "has enough resources for something that's free" do
 report = CostCalculation.new({}).has_resources?({})
 report[:has_resources].should == true
 end 
 %w[wood stone gold].each do |resource| 
 it "does not have enough resources if #{resource} is missing" do
 cost = {resource.to_sym => 1}
 report = CostCalculation.new({wood: 0, stone: 0, gold: 0}).has_resources?(cost)
 report[:has_resources].should == false
 report[:"has_#{resource}"].should == false
 end 
 end 
end

• Martin Frost: Although, one thing that I'd really like to do with this, is that since the method name ends with a question mark, I'd want it to return a boolean, so that it returns false in the cases where the current version returns something with report[:has_resources] == false.
• mu is too short: The only thing I don't like about this is report = Hash.new(true), I'd prefer report[:pancakes] to be nil (which would hopefully cause something to explode and fall over) rather than true. Altering cost might be an issue as well.
• SpyrosP: yeah, the question mark is a remainder actually, from my previous version, which return true or false. The new one should return a hash :)
• Martin Frost: Hm... Now that you mention it, @muistooshort, I actually agree with you. The part of modifying cost can be fixed by just cloning cost and modifying the clone. You could also do something similar with report, or just do report.default = nil before returning it, and also make sure to always set a value for report[:has_resources].

Answer 2

Here is my version: Tested Working under ruby 1.8.7-p334

Assume inputs are:

cost = {:wood => 300, :stone => 200, :gold => 100}
reqs = {:wood => 200, :stone => 220, :gold => 90}

and in the cost hash,:gold, :stone or :wood can be nil.

def has_resources?(cost,reqs)
 result = cost.merge(reqs) { |key, cst, rqs| cst.nil?||(cst - rqs) > 0 ? true : false }
 result[:has_resources] = !result.has_value?(false)
 return result
end

It will return the result of:

{ :has_resources => false, :wood => true, :stone => false, :gold => true } 

If you really want the result to have "has_wood","has_stone","has_gold" as key, you may need to modify the code to rename the key in result Hash. But I think it's not needed.

In case people ask why merge method? here is the Ruby Hash Merge documentation When compare two hash with same key, I think "merge" method is the simple way.

Thanks

Answer 3

First of all, you're repeating yourself through copy'n'paste code so that should be converted to some sort of iteration. Then bust it up into little pieces that can be nicely named and you'll have something compact and readable:

MATERIALS = [ :wood, :stone, :gold ]
def resource_availability(costs)
 MATERIALS.reject(&have_enough_for(costs)).
 each_with_object(base_report, &not_enough) 
end
private
def have_enough_for(costs)
 ->(material) { costs[material].nil? || costs[material] < self.send(material) }
end
def has_sym(material)
 ('has_' + material.to_s).to_sym
end
def base_report
 MATERIALS.each_with_object({ :has_resources => true }) { |m, h| h[has_sym(m)] = true }
end
def not_enough
 ->(material, report) do
 report[has_sym(material)] = false
 report[:has_resources ] = false
 end
end