← Back to Upcase

Refactoring Fast Order Line, with Joe Ferris


(Upcase ) #1
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 https://thoughtbot.com/upcase/videos/refactoring-fast-order-line-with-joe-ferris

(Scott Hollinshead) #2

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 = lines.map do |line|
        line.chomp!
        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
          FastOrderLine.create({
            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,
            user_id: self.current_user.id,
            checkoutref1: location,
            checkoutref3: ref1,
            checkoutref4: ref2
          })
          count +=1
        else
          count +=1
          errors << "There was an error on line #{count}"
          errors << "#{line}"
        end
      end

      if !errors.empty?
        raise ActiveRecord::Rollback
      end

    end

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.


(Ben Orenstein) #3

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 lines.map.with_index 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?

(Scott Hollinshead) #4

@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.


(Ben Orenstein) #5

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 }
  end
end

(Scott Hollinshead) #6

@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.


(Ben Orenstein) #7

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:


(Ben Orenstein) #8

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


(Scott Hollinshead) #9

@benorenstein Yes that’s fine :slight_smile:


(Ben Orenstein) #10

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


(Scott Hollinshead) #11

Awesome, thanks!


(Jacqueline S. Homan) #12

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


(Ben Orenstein) #13

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


(Scott Hollinshead) #14

@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.


(Peter Braswell) #15

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

Line #76: @orders.map(&:save!)

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!


(Aaron Mc Adam) #16

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


(Ben Orenstein) #17

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

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