Refactoring Fast Order Line, with Joe Ferris

Ruby on Rails
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
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.

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

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

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:

1 Like
8

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

1 Like
9

@benorenstein Yes thatā€™s fine :slight_smile:

1 Like
10

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

11

Awesome, thanks!

12

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

2 Likes
13

@scott Itā€™s been released! Hope you find it useful.

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.

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!

16

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

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.