Using update to either update or create new object

I'm using form_remote_for to create a new object in the beginning, and then if the object already exists, update it. I'm using the REST form so my form looks like this:

<% form_remote_for(@spec) do |form| %> <% end %>

with the controller defining the @spec object like so: @spec = (@user.spec ||= Spec.new)

I'm not sure if this is the best way, but my main question lies in the implementation of the update action in the spec controller which is called when the form is submitted. This definitely needs to be refactored and I could use some help with the best way: I also created a pastie, which is much easier to read: http://pastie.org/370302

  def update    respond_to do |format|      if logged_in_user.spec.nil?        @spec = Spec.new(params[:spec])        @spec["user_id"] = logged_in_user.id        if @spec.save           format.js do             render :update do |page|               page.form.reset 'login_form'               page.redirect_to user_path(logged_in_user)             end           end         else           format.js do             render :update do |page|               page << "$('message_popup').popup.show();"             end           end        end      else       if logged_in_user.spec.update_attributes(params[:spec])         format.js do             render :update do |page|               page.redirect_to user_path(logged_in_user)             end           end         else           format.js do             render :update do |page|               page << "$('message_popup').popup.show();"             end           end       end      end    end   end

David wrote:

with the controller defining the @spec object like so: @spec = (@user.spec ||= Spec.new)

That's just || not ||=. Why didn't your unit tests catch that?

I'm not sure if this is the best way, but my main question lies in the implementation of the update action in the spec controller which is called when the form is submitted. This definitely needs to be refactored and I could use some help with the best way: I also created a pastie, which is much easier to read: http://pastie.org/370302

  def update    respond_to do |format|      if logged_in_user.spec.nil?        @spec = Spec.new(params[:spec])        @spec["user_id"] = logged_in_user.id

Why not logged_in_user.spec.build(params[:spec])

       if @spec.save           format.js do

You can move the format.js do line up to just after respond_to. It can wrap all of this...

            render :update do |page|               page.form.reset 'login_form'               page.redirect_to user_path(logged_in_user)

Why are you clearing the form and immediately going to another page?

(Firefox has a "feature" where they preserve form contents. Are you wisely thwarting that?)

            end           end         else           format.js do             render :update do |page|               page << "$('message_popup').popup.show();"

Shouldn't that push the errors into the popup?

            end           end        end      else       if logged_in_user.spec.update_attributes(params[:spec])

Can't this just be part of the new and save bit above? Otherwise all the code below it is the same.

        format.js do             render :update do |page|               page.redirect_to user_path(logged_in_user)             end           end         else           format.js do             render :update do |page|               page << "$('message_popup').popup.show();"             end           end       end      end    end   end

Your unit tests should make that very easy to refactor. Just change one line at a time and pass all the tests after each change!