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!!!
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 G. [email protected]
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 G. [email protected]
Hi –
On Tue, 29 Apr 2008, Becca G. wrote:
controller. I’d be happy to move things around too. Any help on how to
@order.item_codes = nil
@order.item_codes = @fc_string
@order.save
Have 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!
Roy P. 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.
Roy P. 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)
Hi –
On Wed, 30 Apr 2008, Becca G. wrote:
Thanks for the great advice. I started to look at your code reduction
@order.save
@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!
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!
Ach–good catch. Sorry for the bum steer Becca G…
Becca G. 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)
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.