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

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

Maybe this?..

def has_resources?(cost)
    report = { :has_resources => true, :has_wood =>  true, :has_stone => true, :has_gold => true } 

    (self.wood < cost[:wood]) ? report[:has_wood] = report[:has_resources] = false : nil unless cost[:wood].nil?
    (self.stone < cost[:stone]) ? report[:has_stone] = report[:has_resources] = false : nil unless cost[:stone].nil?
    (self.gold < cost[:gold]) ? report[:has_gold] = report[:has_resources] = false : nil unless cost[:gold].nil?
end

Answer 4

What about ?

def has_resources?(cost)
  report = { :has_resources => true, :has_wood =>  true, :has_stone => true, :has_gold => true } 

  report[:has_wood] = report[:has_resources] = (not self.wood < cost[:wood] && report[:has_resources]) if cost[:wood]
  report[:has_stone] = report[:has_resources] = (not self.stone < cost[:stone] && report[:has_resources]) if cost[:stone]
  report[:has_gold] = report[:has_resources] = (not self.gold < cost[:gold] && report[:has_resources]) if cost[:gold]
end

Answer 5

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