3
\$\begingroup\$

I have a method that takes a model object, reads through its children and returns a hash of status counts. It should only return a count on a status if one exists.

This method works, but I was wondering if there are a cleaner, more Rubyish way to write it.

The batch_details table is getting very large, so if possible I'd like not to access it twice.

def batch_status
   batch_status = {
      'ready' => 0,
      'processing' => 0,
      'error' =>  0,
      'purchase' => 0,
      'completed' => 0
   }

   self.batch_details.each do |bd|
      batch_status[bd.status] = batch_status[bd.status] + 1
   end   
   batch_status['count'] = self.batch_details.count
   batch_status.delete_if {|k, v| v == 0 }
   return batch_status
end

2.1.2 :002 > b.batch_status
 => {"completed"=>4, "count"=>4} 
\$\endgroup\$
5
\$\begingroup\$

You can use Array#group_by to produce a hash keyed by status. Then follow that by a map to collect the counts. You'll get an array of key/value pairs, so use Hash[] to make that into a proper hash:

def batch_status
  counts = self.batch_details.group_by(&:status).map { |k, v| [k, v.count] }
  Hash[counts]
end

Alternatively, you can use use your current approach, just shortened a bit

def batch_status
  self.batch_details.reduce({}) do |counts, details|
    counts[details.status] = (counts[details.status] || 0) + 1
  end
end

Edit: Completely overlooked the "count" key. I muddies the picture a little, but you could do something like this for code blocks 1 and 2 above

def batch_status
  counts = self.batch_details.group_by(&:status).map { |k, v| [k, v.count] }
  counts << ["count", self.batch_details.count]
  Hash[counts]
end

and

def batch_status
  counts = { "count" => self.self.batch_details.count }
  self.batch_details.reduce(counts) do |counts, details|
    counts[details.status] = (counts[details.status] || 0) + 1
    counts
  end
end

Or, as Cary Swoveland suggests:

def batch_status
  counts = { "count" => self.self.batch_details.count }
  counts.default = 0
  self.batch_details.reduce(counts) do |counts, details|
    counts[details.status] += 1
    counts
  end
end

Which cleans it up nicely. Further cleanup could be done by using each_with_object in place of reduce:

def batch_status
  counts = { "count" => self.self.batch_details.count }
  counts.default = 0
  self.batch_details.each_with_object(counts) do |counts, details|
    counts[details.status] += 1
  end
end
| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ One possible change to your last batch_status--which I think reads quite well--is to add counts.default = 0 after the first line and then use += 1 in the block. \$\endgroup\$ – Cary Swoveland Jan 8 '15 at 20:00
  • 1
    \$\begingroup\$ @CarySwoveland Good idea - I'll add that \$\endgroup\$ – Flambino Jan 9 '15 at 10:00
1
\$\begingroup\$

This is a variant of @Flambino's second suggestion, which starts with an empty hash, to avoid the step of deleting keys whose values are zero.

def batch_status
  self.batch_details.each_with_object(Hash.new(0)) {|bd,h| batch_status[bd.status] += 1}
                    .merge({"count"=>self.batch_details.count})
end
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Missing closing parenthesis after Hash.new(0). Cannot edit... \$\endgroup\$ – TNT Jan 8 '15 at 19:45
  • \$\begingroup\$ Thanks, TNT. I thought I had fixed that, but it's fixed now. \$\endgroup\$ – Cary Swoveland Jan 8 '15 at 20:03

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.