Rails best practice: conditional action, multiple actions, or method?

I'm curious to get some input on a chunk of code I've been working on recently.

I have a model, photos, which sometimes (but not always) belong_to a collection. I have a page for users to manage a collection, they can add any unassigned photos to the collection or remove photos from the collection.

This is an "edit multiple" situation, so I created two new controller actions: select, which handles the GET request and the view, and assign which handles the PUT request from the checkboxes in the select view.

Because the user can be either adding photos to a collection or removing photos from a collection, my assign action has a condition in it, and it looks like this:

def assign
    @photos = Photo.find(params[:photo_ids])
    case params[:assignment]
    when 'add'
        @photos.each do |p|
            p.collection = @collection
            p.save!
        end
        notice = "Photos added to collection."
    when 'remove'
        @photos.each do |p|
            p.collection = nil
            p.save!
        end
        notice = "Photos removed from collection."
    end
    redirect_to select_collection_photos_path(@collection), :notice => notice
end

This works exactly as expe开发者_Python百科cted. However, I feel uncomfortable with it, it doesn't seem to fit the "Rails Way."

Other Rails developers, when you have this kind of situation, would you handle it as I have? Would you split this across two controller actions (ie add_to_collection and remove_from_collection), or would you move it to the model? If you were to move it to the model, what would that look like?

I'd appreciate any suggestions and feedback. Thanks!

There's probably a few different ways you could refactor this, but the most obvious one seems to be moving all the photo logic to the Photo model. Even though this is your photos controller, it shouldn't know that much about the Photo model.

I would probably do something along these lines in your controller:

def assign
  Photo.update_collection(params, @collection)

  redirect_to select_collection_photos_path(@collection), :notice => "Photo collection updated"
end

Then in your Photo model:

class Photo < ActiveRecord::Base
  def self.update_collection(params, collection)

    photos = Photo.find(params[:photo_ids])

    case params[:assignment]
    when 'add'
      photos.each {|p| p.add_collection(collection) }
    when 'remove'
      photos.each {|p| p.remove_collection }
    end
  end

  def add_collection(collection)
    self.collection = collection
    save!        
  end

  def remove_collection
    self.collection = nil
    save!
  end
end

Breaking the functionality up into smaller model methods makes it easier for unit testing, which you should be doing if you're not :)

This is actually a prime candidate for accepts_nested_attributes_for.

Instead of thinking about new actions in the controller, stick to the standard REST conventions whenever possible. Excepting fancy UI display stuff (like your select action), very rarely do I find that I need to deviate from the standard CRUD actions present in a generated scaffold_controller.

If you set accepts_nested_attributes_for :collection in your Photo model, you should be able to build up a special form that assigns collections to photos. I won't go into the full details here, but will instead point you to http://railscasts.com/episodes/196-nested-model-form-part-1 and http://railscasts.com/episodes/197-nested-model-form-part-2 . It'll be more work in the view, but you'll come out far ahead in more simple, easily testable controllers and models.