I''ve got a field in a database that contains a string of ids.
I want to be able to remove an item from that list and save the list
again. The only way I saw how to do this was to convert it to an array
and back to a string again.
The variable names are for an example.
This code works, but I don''t think that it''s efficient.
It''s all in my
controller. I''d be happy to move things around too. Any help on how
to
do this much better would be appreciated.
@order = Order.find(params[:id])
@remove_item = params[:item]
@item_array = @order.item_codes.split(/,\s*/)
@item_array.delete(@remove_item)
@item_string = @fc_array.join(",")
@order.item_codes = nil
@order.item_codes = @fc_string
@order.save
THANKS!!!
--
Posted via http://www.ruby-forum.com/.
--~--~---------~--~----~------------~-------~--~----~
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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---
I''m torn between my "don''t use compound fields" and "don''t optimize prematurely" response templates here. ;-). I think I remember you saying a while ago that you couldn''t un-compound the field here, so... How confident are you that this is posing a performance problem for you? On Apr 29, 2:44 pm, Becca Girl <rails-mailing-l...-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org> wrote:> I''ve got a field in a database that contains a string of ids. > > I want to be able to remove an item from that list and save the list > again. The only way I saw how to do this was to convert it to an array > and back to a string again. > > The variable names are for an example. > > This code works, but I don''t think that it''s efficient. It''s all in my > controller. I''d be happy to move things around too. Any help on how to > do this much better would be appreciated. > > @order = Order.find(params[:id]) > @remove_item = params[:item] > > @item_array = @order.item_codes.split(/,\s*/) > @item_array.delete(@remove_item) > @item_string = @fc_array.join(",") > @order.item_codes = nil > @order.item_codes = @fc_string > @order.save > > THANKS!!! > -- > Posted viahttp://www.ruby-forum.com/.--~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
Roy Pardee wrote:>How confident are you that this is posing a performance problem > for you?This is an internal application that runs pretty fast. I''m essentially just looking at ways to create as clean of code as possible and am seeking best practices. This really is code that''s cobbled together that works. Now that I have it working, I can give people an idea of the concept and ask how it should look given the constraints that I have. Thanks. -- Posted via http://www.ruby-forum.com/. --~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
Well FWIW, I don''t see anything really egregious in the code there--
the split/delete/join strategy seems an obvious one to use. Maybe
someone else w/more insight will chime in.
I expect you could leave this line out w/out any ill effect:
@order.item_codes = nil
You could also be using plain method_vars instead of @instance_vars
for most of those, tho I don''t know that there are efficiency
considerations to choosing one or the other in this context.
You could make most of it a one-liner by chaining method calls
together:
@order.item_codes = @order.item_codes.split(/,
\s*/).delete(params[:item]).join(",")
That saves some variable allocations, tut it''s not nearly as pretty as
what you''ve got.
You could also probably gsub!() your unwanted item out of there w/out
doing the split/join, but there too you''ll sacrifice readability.
I say leave it alone. ;-)
HTH,
-Roy
On Apr 29, 3:05 pm, Becca Girl
<rails-mailing-l...-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org>
wrote:> Roy Pardee wrote:
> >How confident are you that this is posing a performance problem
> > for you?
>
> This is an internal application that runs pretty fast. I''m
essentially
> just looking at ways to create as clean of code as possible and am
> seeking best practices. This really is code that''s cobbled
together
> that works. Now that I have it working, I can give people an idea of
> the concept and ask how it should look given the constraints that I
> have.
>
> Thanks.
> --
> Posted viahttp://www.ruby-forum.com/.
--~--~---------~--~----~------------~-------~--~----~
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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
For more options, visit this group at
http://groups.google.com/group/rubyonrails-talk?hl=en
-~----------~----~----~----~------~----~------~--~---
Hi -- On Tue, 29 Apr 2008, Becca Girl wrote:> > I''ve got a field in a database that contains a string of ids. > > I want to be able to remove an item from that list and save the list > again. The only way I saw how to do this was to convert it to an array > and back to a string again. > > The variable names are for an example. > > This code works, but I don''t think that it''s efficient. It''s all in my > controller. I''d be happy to move things around too. Any help on how to > do this much better would be appreciated. > > > > @order = Order.find(params[:id]) > @remove_item = params[:item] > > @item_array = @order.item_codes.split(/,\s*/) > @item_array.delete(@remove_item) > @item_string = @fc_array.join(",") > @order.item_codes = nil > @order.item_codes = @fc_string > @order.saveHave you considered using a serialized attribute? That will automatically convert back and forth. David -- Rails training from David A. Black and Ruby Power and Light: INTRO TO RAILS June 9-12 Berlin ADVANCING WITH RAILS June 16-19 Berlin INTRO TO RAILS June 24-27 London (Skills Matter) See http://www.rubypal.com for details and updates! --~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
Roy Pardee wrote:> @order.item_codes = @order.item_codes.split(/, > \s*/).delete(params[:item]).join(",") > > That saves some variable allocations, tut it''s not nearly as pretty as > what you''ve got.Thanks for the great advice. I started to look at your code reduction and went from this: @order = Order.find(params[:id]) @remove_item = params[:item] @item_array = @order.item_codes.split(/,\s*/) @item_array.delete(@remove_item) @item_string = @fc_array.join(",") @order.item_codes = nil @order.item_codes = @fc_string @order.save To this: @order = Order.find(params[:id]) @order.item_codes = @order.item_codes.split(/, \s*/).delete(params[:item]).join(",") @order.save I must have taken out too much?? I get this error: NoMethodError (You have a nil object when you didn''t expect it! You might have expected an instance of Array. The error occurred while evaluating nil.join) -- Posted via http://www.ruby-forum.com/. --~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
Hi -- On Wed, 30 Apr 2008, Becca Girl wrote:> > Roy Pardee wrote: > >> @order.item_codes = @order.item_codes.split(/, >> \s*/).delete(params[:item]).join(",") >> >> That saves some variable allocations, tut it''s not nearly as pretty as >> what you''ve got. > > > Thanks for the great advice. I started to look at your code reduction > and went from this: > > @order = Order.find(params[:id]) > @remove_item = params[:item] > > @item_array = @order.item_codes.split(/,\s*/) > @item_array.delete(@remove_item) > @item_string = @fc_array.join(",") > @order.item_codes = nil > @order.item_codes = @fc_string > @order.save > > > > To this: > > @order = Order.find(params[:id]) > > @order.item_codes = @order.item_codes.split(/, > \s*/).delete(params[:item]).join(",") > > @order.save > > > I must have taken out too much?? I get this error: > > NoMethodError (You have a nil object when you didn''t expect it! > You might have expected an instance of Array. > The error occurred while evaluating nil.join)delete will return the object you''ve deleted, which means in this case the array did not contain params[:item]. In any case, you don''t want to call join on that one item, even it is there. There are ways to chain a deletion like that but I''d actually unroll it a bit -- maybe like this: @order = Order.find(params[:id]) item_array = @order.item_codes.split(/,\s*/) if item_array.include?(@params[:item]) item_array.delete(@params[:item]) @order.item_codes = item_array.join(",") @order.save end David -- Rails training from David A. Black and Ruby Power and Light: INTRO TO RAILS June 9-12 Berlin ADVANCING WITH RAILS June 16-19 Berlin INTRO TO RAILS June 24-27 London (Skills Matter) See http://www.rubypal.com for details and updates! --~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
Ach--good catch. Sorry for the bum steer Becca Girl... On Apr 30, 9:41 am, "David A. Black" <dbl...-0o/XNnkTkwhBDgjK7y7TUQ@public.gmane.org> wrote:> Hi -- > > > > On Wed, 30 Apr 2008, Becca Girl wrote: > > > Roy Pardee wrote: > > >> @order.item_codes = @order.item_codes.split(/, > >> \s*/).delete(params[:item]).join(",") > > >> That saves some variable allocations, tut it''s not nearly as pretty as > >> what you''ve got. > > > Thanks for the great advice. I started to look at your code reduction > > and went from this: > > > @order = Order.find(params[:id]) > > @remove_item = params[:item] > > > @item_array = @order.item_codes.split(/,\s*/) > > @item_array.delete(@remove_item) > > @item_string = @fc_array.join(",") > > @order.item_codes = nil > > @order.item_codes = @fc_string > > @order.save > > > To this: > > > @order = Order.find(params[:id]) > > > @order.item_codes = @order.item_codes.split(/, > > \s*/).delete(params[:item]).join(",") > > > @order.save > > > I must have taken out too much?? I get this error: > > > NoMethodError (You have a nil object when you didn''t expect it! > > You might have expected an instance of Array. > > The error occurred while evaluating nil.join) > > delete will return the object you''ve deleted, which means in this case > the array did not contain params[:item]. In any case, you don''t want > to call join on that one item, even it is there. > > There are ways to chain a deletion like that but I''d actually unroll > it a bit -- maybe like this: > > @order = Order.find(params[:id]) > item_array = @order.item_codes.split(/,\s*/) > if item_array.include?(@params[:item]) > item_array.delete(@params[:item]) > @order.item_codes = item_array.join(",") > @order.save > end > > David > > -- > Rails training from David A. Black and Ruby Power and Light: > INTRO TO RAILS June 9-12 Berlin > ADVANCING WITH RAILS June 16-19 Berlin > INTRO TO RAILS June 24-27 London (Skills Matter) > Seehttp://www.rubypal.comfor details and updates!--~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
David A. Black wrote:> > @order = Order.find(params[:id]) > item_array = @order.item_codes.split(/,\s*/) > if item_array.include?(@params[:item]) > item_array.delete(@params[:item]) > @order.item_codes = item_array.join(",") > @order.save > end >David - Thanks so much for the final solution. This is exactly what I was looking for. Roy, thanks for getting us started. You guys are great! -- Posted via http://www.ruby-forum.com/. --~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
Becca Girl wrote:> David - Thanks so much for the final solution. This is exactly what I > was looking for.One other thing is that it may be worth capturing this behaviour in a separate method. That will make your main method scan easier and keep the implementation details out of the way and available for reuse. If it is applicable only for these order item_codes then perhaps a method #remove_item_code in the Order model, else (dare I say it) monkey patch String with a method to manage these lists. (ducks) -- Posted via http://www.ruby-forum.com/. --~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---
I very much agree with the recommendation to move this to the model. Not only can you reuse the code from other places, but encapsulating it will allow you to change your mind later. For example, you could change to a has_many solution rather than a set of ids in a string (or even a serialized array). With the methods for inserting and removing from the collection encapsulated in the model you only have to change the model with the (business logic) code out in the controllers you have to find each occurrence of deletion-like code and update it. On May 1, 3:52 am, Mark Bush <rails-mailing-l...-ARtvInVfO7ksV2N9l4h3zg@public.gmane.org> wrote:> Becca Girl wrote: > > David - Thanks so much for the final solution. This is exactly what I > > was looking for. > > One other thing is that it may be worth capturing this behaviour in a > separate method. That will make your main method scan easier and keep > the implementation details out of the way and available for reuse. > > If it is applicable only for these order item_codes then perhaps a > method #remove_item_code in the Order model, else (dare I say it) monkey > patch String with a method to manage these lists. (ducks) > > -- > Posted viahttp://www.ruby-forum.com/.--~--~---------~--~----~------------~-------~--~----~ 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-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org For more options, visit this group at http://groups.google.com/group/rubyonrails-talk?hl=en -~----------~----~----~----~------~----~------~--~---