How can I grow this search object?

I've got a domain object I use to search images by whatever tags it has/the year the item in the image was made. It's worked flawlessly in a couple places:

class SearchImage

  def initialize(params)
    @params = params
    @tags = params[:tags] unless params.has_key?(:tags) && params[:tags].empty?
    @year = params[:year] unless params.has_key?(:year) && params[:year].empty?
  end

  def search
    if @params[:commit] == "All Uncategorized"
      unfinished
    else
      sort
    end
  end

  def sort
    @images = Image.tagged_with(@tags) if @tags
    @images = @images.where(year: @year) if @year

    @images ||= default_image_search
  end

  def unfinished
    @images = Image.unfinished
  end

  def default_image_search
    Image.order(created_at: :desc).first(8)
  end
end

But now, I need to filter the results to only return images that aren't in a given collection. The controller action currently looks like this.

def choices
  @images = SearchImage.new(params).search
end

and the way I can get the images I don't want to include in the results is:

@collection.images

Since this is just another way of filtering the search, I figure I could just check for the existence of a collection_id in the params and subtract out the currently selected images from the result...but now, I feel everything is becoming tightly coupled to the state of the page and I'll be adding conditionals of some type to check for the existence, and act upon that existence.

Or I could inject a filter that does that at the end, but that doesn't seem right either.

So I'm gonna go take a nap and then get back on this, but in the meantime here's my query: how could I get rid of the if statement in the `search' function and instead start telling it what to do? Or, if thats not really a good question, what's a different way I could think about the objects and associated responsibilities/actual execution in ruby?

What are the collection_id and selected images parts? I could use a little more context to determine what I'd suggest but I don't think you're far from a good solution.

A collection is simply an ordered list of images that will appear wherever its wanted (in a carousel, in a gallery, etc). Selection is basically a join table that sits between a collection & its images, both so I can add a position column and have an image belong to as many different collections as needed.

class Collection < ActiveRecord::Base
  has_many :selections, -> { order(position: :asc) }
  has_many :images, through: :selections
  accepts_nested_attributes_for :selections, allow_destroy: true
end

class Selection < ActiveRecord::Base
  belongs_to :collection
  belongs_to :image
  acts_as_list scope: :collection
end

class Image < ActiveRecord::Base
  attachment :file
  belongs_to :image_category
  has_many :selections
  acts_as_taggable

  scope :unfinished, -> { where( year: nil) }

  def self.unfinished_count
    unfinished.count
  end
end

If I'm on a page where there's a collection being displayed for editing, the params[:collection_id] will be present, so then I can include & treat that like another filtering component.

class SearchImage

  def initialize(params)
    @params = params
    @tags = params[:tags] unless params.has_key?(:tags) && params[:tags].empty?
    @year = params[:year] unless params.has_key?(:year) && params[:year].empty?

    set_collection if params.has_key?[:collection_id] && !params[:collection_id].empty?
  end

  def search
    if @params[:commit] == "All Uncategorized"
      unfinished
    else
      sort
    end
  end

  def sort
    @images = Image.tagged_with(@tags) if @tags
    @images = @images.where(year: @year) if @year

    @images ||= default_image_search
  end

  def set_collection
    @collection = Collection.find(params[:collection_id])
  end

  def unfinished
    @images = Image.unfinished
  end

  def default_image_search
    Image.order(created_at: :desc).first(8)
  end

This honestly seems like I could just be chaining queries in some fashion, meaning I could add in:

Image.where.not(id: @collection.images.pluck(:id))

or something, but where I'm stuck is: how could I get rid of that if so don't need to deal with two branching paths? Just have one path that will do the right thing. Search and sort don't seem easy to change

Thus far, this is my best take on it:

class SearchImage

  def initialize(params)
    @params = params
    @tags = params[:tags] unless params.has_key?(:tags) && params[:tags].empty?
    @year = params[:year] unless params.has_key?(:year) && params[:year].empty?

    set_excluded_image_ids if params.has_key?(:collection_id) && !params[:collection_id].empty?
  end

  def search
    if @params[:commit] == "All Uncategorized"
      unfinished
    else
      sort
    end
  end

  def sort
    @images = Image.excluding(@excluded_image_ids).tagged_with(@tags) if @tags
    @images = @images.excluding(@excluded_image_ids).where(year: @year) if @year

    @images ||= default_image_search
  end

  def set_excluded_image_ids
    collection = Collection.find(@params[:collection_id])
    @excluded_image_ids = collection.images.pluck(:id)
  end

  def unfinished
    @images = Image.excluding(@excluded_image_ids).unfinished
  end

  def default_image_search
    Image.excluding(@excluded_image_ids).order(created_at: :desc).first(8)
  end
end

class Image < ActiveRecord::Base
  def self.excluding(ids)
    where.not(id: ids)
  end
end

it works, for now, but I have this feeling I'm not discovering some deeper pattern made available by ruby that I don't see

There's actually a lot going on here and I can see several bugs because there is logic smattered all over. My impression is that you've extracted lines of code into their own methods too early before you knew exactly what you were trying to accomplish. That left you with a bunch of methods and several of them having their own little bits of logic. There's no clear path that anything flows which makes it hard to understand at a glance.

I think you have felt this and recognized it, so the question is how do I get back to sanity? And the answer is by making your code uglier. Remove all these method calls and put the code back inline. You only call most of these methods from a single place so removing the methods and putting them back inline clarifies things a lot. You can then see what's duplicated. Make the search method contain all the logic and then refactor it.

From what I can tell you always want to exclude the image ids, yet you do this in many different places so it's hard to realize that. Your search method could actually start by calling Image.excluding first and then tack on the other scopes instead. I'd do a refactoring myself and maybe a video of it if I had the time, but hopefully that makes sense as an approach to start refactoring.

Update:

I was trying to create an array of blocks which contained queries that would lazily evaluate until I realized these are scopes, and they already exist. some of these queries aren't pretty, but for the usage they'll be under thats totally fine.

class Image < ActiveRecord::Base
  scope :unfinished, -> { tagged_with(ActsAsTaggableOn::Tag.all.map(&:to_s), exclude: true).where(year: nil) }
  scope :excluding, -> (ids) { where.not(id: ids) if ids.present? }
  scope :by_tags, -> (tags) { tagged_with(tags) if tags.present? }
  scope :by_year, -> (year) { where(year: year) if year.present? }
  scope :default, -> {order(created_at: :desc).first(8)}
  scope :none_if_all, -> {where(id: nil).where("id IS NOT ?", nil) if all.count == Image.all.count }
end

class SearchImage

  def initialize(params)
    @params = params
    @tags = params[:tags] || ""
    @year = params[:year] || ""

    set_excluded_image_ids if params.has_key?(:collection_id) && !params[:collection_id].empty?
  end

  def search
    if @params[:commit] == "View Uncategorized Images"
      @images = Image.excluding(@excluded_image_ids).unfinished
    elsif @tags.empty? && @year.empty?
      @images = Image.excluding(@excluded_image_ids).default
    else
      @images = Image.excluding(@excluded_image_ids).by_tags(@tags).by_year(@year).none_if_all
    end
  end

  private

  def set_excluded_image_ids
    collection = Collection.find(@params[:collection_id])
    @excluded_image_ids = collection.images.pluck(:id)
  end
end

That's much better! I'd clean this up too slightly since you're repeating the exclude every time.

  def search
    @images = Image.excluding(@excluded_image_ids)
    if @params[:commit] == "View Uncategorized Images"
      @images.unfinished
    elsif @tags.empty? && @year.empty?
      @images.default
    else
      @images.by_tags(@tags).by_year(@year).none_if_all
    end
  end