Refactoring Fast Order Line, with Joe Ferris

In this episode, Ben and Joe refactor some code submitted by a subscriber in our forum. Starting with a controller action containing complex business logic adding up to more than fifty lines, they extract a class to encapsulate that logic, using ...
This is a companion discussion topic for the original entry at

How it works

A user will receive a txt file every morning, they then copy this text and paste it into a text_area when they hit submit it adds all these lines into the cart.

What I have so far

So far I have managed to split all the required information up and insert it into the relavent fields in the form.

This is some sample data from the text file

F1234/0/1|11-87-17|-1|JOHN DOE|COMPANY
F1234/0/1|11-87-17|1|JOHN DOE|COMPANY
F1234/0/1|11-87-38|1|JOHN DOE|COMPANY
F1234/0/1|12-22-87|1|JANE DOE|COMPANY

This is the code that I have at the moment that is working.

def text_input2
    lines = params[:template1].lines
    errors = []
    count = 0
    all_rows = []

    FastOrderLine.transaction do
      orders = do |line|
        regex = /^(.+\/\d+\/\d+)\|(\d\d-\d\d-\d\d)\|(-?\d+)\|([^|]+)\|([^|]+)$/
        if line.match(regex)
          line = line.match(regex).to_a
          all, location, prnum, qty, ref1, ref2 = line
            entered_prnum: prnum,
            qty: qty,
            vat_rate: Cart.default_vat_rate,
            line_vat: 0,
            product_id: 0,
            price: 0.0,
            selling_unit: 1,
            special_id: 0,
            checkoutref1: location,
            checkoutref3: ref1,
            checkoutref4: ref2
          count +=1
          count +=1
          errors << "There was an error on line #{count}"
          errors << "#{line}"

      if !errors.empty?
        raise ActiveRecord::Rollback


The Problem

I need to balance out the negative numbers, so as you can see the first line has a quantity of -1 but then the next line from the same person is 1, so I would like to somehow merge these together where the lines are identical apart from their quantites. Then at the end if any of the quantities are 0 they get removed.

Any help is much appreciated.

I’d consider building up a collection (possibly a hash) of Orders from your text file.

For each line, see if there’s an order in the collection already. If there is, add the quantity from the text file to the quantity in the collection. When you’re done processing the text file, reject any orders whose quantity is 0.

Make sense?

Also, since you pasted code, I’m going to review it. :smile:

  1. I’d be looking to extract methods from this until none of the methods involved are longer than a line or two. This method is far too long to be understood easily and you haven’t even pasted all of it.
  2. The method name doesn’t seem very clear.
  3. You probably want to call create! so this doesn’t silently fail to create FastOrderLines.
  4. You’re incrementing count in both branches of your conditional. That means you can do it outside the conditional.
  5. Rather than manually using count at all, consider do |line, index|
  6. all, location, prnum, qty, ref1, ref2 = line scares me. Are you sure you need six additional references to line?
  7. Ironically, regex isn’t a particularly good name for a regex, since we can already see it’s a regex. Can you choose a name that gives the reader more of a clue of what the regex is used for?

@benorenstein Thank you.

I am not sure on how to do the above, I have made all the changes you have mentioned in the list apart from putting it all into smaller methods as I don’t see how I can do that until I know how to build the collection.

Here’s a spec for roughly what I mean.

describe '#count_orders' do 
  it 'creates a hash of order ids to quantities' do
    order_1 =          "F1234/0/1|11-87-17|1|JOHN DOE|COMPANY"
    order_1_reversal = "F1234/0/1|11-87-17|-1|JOHN DOE|COMPANY"
    order_2 =          "F1234/0/1|11-87-38|1|JOHN DOE|COMPANY"

    result = count_orders([order_1, order_1_reversal, order_2])

    result.should eq { "11-87-17" => 0, "11-87-38" => 1 }

@benorenstein That makes more sense now thanks, I need to find the order where F1234/0/1 and 11-87-17 and JOHN DOE are the same, and if it exists update the quantity.

You will probably be horrified by this code, but I have put what I have come up with in this gist as like you said it is huge.

Anything you could point out to make this cleaner would be a big help, I am trying to move it into a class but it’s the first time I am doing this and it is proving difficult, I have just bought practical object oriented programming in ruby to try and help me understand this concept more.

Thanks for your help @benorenstein, sorry if it seems like I have wasted your time.

Ruby Science has a section on Extract Class that would probably help.

However, I’d start with just breaking this down into smaller methods. It will make what you’re doing much clearer.

P.S. You haven’t wasted my time! This is pretty much my job. :smile:

1 Like

I’m considering refactoring this code as a Weekly Iteration episode. Would that be okay with you?

1 Like

@benorenstein Yes that’s fine :slight_smile:

1 Like

We recorded the episode yesterday. It should come out on Monday.

Awesome, thanks!

Every time I read the convos here on the forum, I feel like my coder IQ jumps 50 more points :smile:


@scott It’s been released! Hope you find it useful.

@benorenstein Thanks, I watched it on Monday, it was extremley helpful thanks! I am starting to apply the same techniques to other big controller actions now too :D.

Really nice refactoring walkthrough. I really like the “over-the-shoulder” approach.

Line #76:!)

Had me scratching my head for a bit. I’d never seen that type of syntax before. For the benefit of others a little research reveals that the ‘$’ calls the #to_proc method on the class and turns it into a Proc which is what map is expecting.

Very cool!

I know this post is old, but regards point 6, isn’t that line using Ruby’s implicit splatting to deconstruct the array?

Yep, you’re right. I misread this when reviewing it.

Still, I’d say that splatting out 6 args is still a bit fishy.