Better way to remove value from list??

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. :wink:

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.