Best Practice: Delete Method

Rails
1

Hey,
I was just wondering what the best practice for this situation would
be. I’ve got two models(word, definition) both with destroy methods in
the controllers that just change the model.status to “deleted”. In the
words controller I’d like it to call the definition_controller destroy
method on definition models. And I’m just blanking on how to do this
like a regular model method. Would I have to double the destory method
in the model? Then I wouldn’t be adhering to dry.

// words_controller.rb
def destroy
begin
ActiveRecord::Base.transaction do
@word = Word.find(params[:id], :include => :definitions)
@word.status = ‘deleted’
@word.save

    @word.definitions.each do |h|
      h.destroy // Actually destorys the record instead of just

setting the status to “deleted”
end
end
rescue ActiveRecord::RecordInvalid => invalid
flash[:notice] = ‘Word was not deleted’
render :action => “new”
end

respond_to do |format|
  format.html { redirect_to(words_url) }
end

end

// definitions_controller.rb
def destroy
begin
ActiveRecord::Base.transaction do
@definition = Definition.find(params[:id])
@definition.status = ‘deleted’
@definition.save
end
rescue ActiveRecord::RecordInvalid => invalid
flash[:notice] = ‘Definition was not deleted’
render :controller => :words, :action => “new”
end

respond_to do |format|
  format.html { redirect_to(words_url) }
end

end

Thanks,
bp

2

On Sep 5, 3:07 pm, brianp [email protected] wrote:

def destroy
end
// definitions_controller.rb
end

respond_to do |format|
  format.html { redirect_to(words_url) }
end

end

Thanks,
bp

I’m not sure how creating a method that sets the status attribute to
“deleted” and saves the record would conflict with DRY principles?
Also, it seems that the specifics of this what-would-be “mark_deleted”
or maybe “set_status(‘deleted’)” method is something the controller
probably doesn’t care about; more the controller just wants your
model to perform a specific unit of logic.

Moreover, it doesn’t make sense (as far as I can see) to call a
separate controller’s actions outside the realm of the HTTP redirect
dance. Controllers exist to sit in between your user’s request and the
view/content that they want rendered back, having the appropriate
models perform whatever actual logic is required to make this happen.

3

Your are very right. I shouldn’t actually have the controller changing
the status at all. I should maybe have the controller call the models
change status method when called but that is it. And because the
method will now be in the model it wont be in the controller and I
wont be breaking dry like I thought of before.

I should have thought of that earlier once i realized destroy was
doing a little more work then actually destroying.

Thanks for the input, I knew I was looking at it from the wrong angle
it just felt wrong.

4

2009/9/5 brianp [email protected]:

So after what i end up with is a:

// word.rb, definition.rb
 def set_status(status)
  self.status = status
 end

Arguably set_status is not a good name, as it requires the caller to
know about the status field in the model. Something mark_deleted
might be a better name.

Colin

5

So after what i end up with is a:

// word.rb, definition.rb
def set_status(status)
self.status = status
end

// word_controller.rb
def destroy
@word = Word.find(params[:id], :include => :definitions)

begin
  ActiveRecord::Base.transaction do
    @word.set_status("deleted")
    @word.save

    @word.definitions.each do |h|
      h.set_status("deleted")
      h.save
    end
end
rescue ActiveRecord::RecordInvalid => invalid
    flash[:notice] = 'Word was not deleted'
    render :action => "new"
end

respond_to do |format|
  format.html { redirect_to(words_url) }
end

end

Which seems much more appropriate.

6

Moreover, can’t we move out explicit save in the controller to model
like
this:

model.rb

def mark_deleted
self.update_attribute(:status, “deleted”)
end

Thanks,
Abhinav

–
अभिनव
http://twitter.com/abhinav