How to improve this piece of code

Question

I am running an online handbag store where handbags can be of four colors - black, brown, orange and red. I have notice that black handbags sell sooner than brown handbags and so forth. That means people like black handbags the most.

On the homepage of the online store I want to select and display 10 bags in a grid layout. So I start by selecting black bags. If I have 10 or more black bags in my inventory then I stop and don't look for the rest of the bags of other colors. However, if I have 5 black bags then I would continue to look for brown bags. After adding those brown bags if I still don't have 10 bags then I look for orange bags and so forth.

Below is my attempt at implementing the solution as Rails model methods:

class Handbag < ActiveRecord::Base
  belongs_to :store
  attr_accessor :color
end

class Store < ActiveRecord::Base
  has_many :handbags

  def handags_for_display
    selected_handbags = []
    ["black", "brown", "orange", "red"].each do |color|
      bags = get_handbags_by_color(color)
      selected_bags += bags
      if selected_bags.size >= 10
        selected_handbags = selected_handbags[0..9]
        break
      end
    end
    selected_handbags
  end

  private
  def get_handbags_by_color(color)
    handbags.where("color = ?", color).limit(10)
  end
end

Though this works, I am curious if there is a better way to write it. Particularly, I think this code can be converted to use Ruby's Enumerator.

Answer 1

You should just query the database at once doing something like:

@page_offset = ((params[:page].to_i-1)*10) || 0
Handbag.order("color ASC").limit(10).offset(@page_offset)

Fortunately the colors already happen to be in alphabetical order.

Answer 2

You could try a recursive function like this. This works as expected (running this will give you {:black => 1, :brown => 8, :orange => 1}), and you can just change the get_handbags_by_color to work with Rails instead.

@bags = {
  :black => 1,
  :brown => 8,
  :orange => 10,
  :red => 10
}

@handbag_order = [:black, :brown, :orange, :red]
@max_bags = 10

def get_handbags_by_color(color,limit)
  num = @bags[color]
  num > limit ? limit : num
end

def handbags_for_display(index = 0, total = 0)
  color = @handbag_order[index]
  return {} unless color
  handbags = {color => get_handbags_by_color(color,@max_bags - total)}
  total += handbags.values.inject{|sum,x| sum+x}

  handbags.merge!(handbags_for_display(index+1, total)) unless(total >= @max_bags)
  handbags
end

handbags = handbags_for_display

Answer 3

def handags_for_display
     handbags = Array.new
     [{ :color => 'black', :count => 5 }, { :color => 'brown' , :count => 2 }, { :color => 'orange', :count => 1 }, { :color => 'red', :count => 1}].each do |handbag|
          handbags+=Handbag.where("color = ?", handbag[:color]).limit(handbag[:count])
     end
     handbags
end