thr3ads.net - Rails - Best Practice: Delete Method [Sep 2009]

If this information is useful, please help other people find it:
Share via:

brianp

2009-Sep-05 19:07 UTC

Best Practice: Delete Method

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

pharrington

2009-Sep-05 19:25 UTC

head link

Re: Best Practice: Delete Method

On Sep 5, 3:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:> 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
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.

brianp

2009-Sep-05 20:07 UTC

head link

Re: Best Practice: Delete Method

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.

On Sep 5, 12:25 pm, pharrington
<xenogene...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:> On Sep 5, 3:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>
>
> > 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
>
> 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.

brianp

2009-Sep-05 20:37 UTC

head link

Re: Best Practice: Delete Method

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.


On Sep 5, 1:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
wrote:> 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.
>
> On Sep 5, 12:25 pm, pharrington
<xenogene...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > On Sep 5, 3:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> > > 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
>
> > 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.

Colin Law

2009-Sep-06 07:26 UTC

head link

Re: Best Practice: Delete Method

2009/9/5 brianp
<brian.o.pearce-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:>
> 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
>
> // word_controller.rb
>  def destroy
>    @word = Word.find(params[:id], :include => :definitions)
>
>    begin
>      ActiveRecord::Base.transaction do
>        -ZARPe5Ea0C8@public.gmane.org_status("deleted")
>        -6vl0TextgHezQB+pC5nmwQ@public.gmane.org
>
>        -el46kIj/u7+2F5/M12p16dxF6Il8aaE1@public.gmane.org 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.
>
>
> On Sep 5, 1:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> 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.
>>
>> On Sep 5, 12:25 pm, pharrington
<xenogene...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > On Sep 5, 3:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > > 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
>>
>> > 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.
> >
>

Abhinav Saxena

2009-Sep-07 10:33 UTC

head link

Re: Best Practice: Delete Method

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



On Sun, Sep 6, 2009 at 12:56 PM, Colin Law
<clanlaw-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
>
> 2009/9/5 brianp
<brian.o.pearce-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >
> > 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
>
> >
> > // 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.
> >
> >
> > On Sep 5, 1:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >> 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.
> >>
> >> On Sep 5, 12:25 pm, pharrington
<xenogene...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> > On Sep 5, 3:07 pm, brianp
<brian.o.pea...-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >>
> >> > > 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
> >>
> >> > 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.
> > >
> >
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Talk" group.
To post to this group, send email to
rubyonrails-talk-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
To unsubscribe from this group, send email to
rubyonrails-talk+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---

Rails - Sep 2009 - Best Practice: Delete Method

Best Practice: Delete Method

Re: Best Practice: Delete Method

Re: Best Practice: Delete Method

Re: Best Practice: Delete Method

Re: Best Practice: Delete Method

Re: Best Practice: Delete Method