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?
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.
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
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)
Hi --
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
Ach--good catch. Sorry for the bum steer Becca Girl...
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!
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)
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.