モデルに大きなコントローラメソッドをリファクタリングする

Question

私はphpコードベースから移植されたレールアプリを持っています。 私は基本的にカート内のアイテムに基づいて合計価格を計算する長いコントローラメソッドを持っています。これは、PHPコードから直接移植される従来の方法です。上記のコード

def total 
    order = @cart.get_or_create_order 
    order_contents = order_contents_for(order) 

    discounts = { 
     events: {}, 
     subjects: {}, 
     products: {} 
    } 

    adjusted_pricing = {} 
    free = false 
    shipping = 0 
    total = 0 

    # Logic for computing shipping price 

    # Construct discount hash 

    order_contents.each do |item| 
     if (item.variant_price.present?) 
     price = item.variant_price 
     end 
     else 
     price = item.price 
     end 

     price_adjustments = {} 
     popped_from = [] 

     # it's the issue due to legacy database structure, 
     # product_id, subject_id and event_id is each column in 
     # the database 
     if (discounts[:products][item.product_id]) 
      price_adjustments = discounts[:products][item.product_id] 
      discounts[:products].delete(item.product_id) 
      popped_from = [:products, item.product_id] 
     elsif (discounts[:subjects][item.subject_id]) 
      price_adjustments = discounts[:subjects][item.subject_id] 
      discounts[:subjects].delete(item.subject_id) 
      popped_from = [:subjects, item.subject_id] 
     elsif (discounts[:events][item.event_id]) 
      price_adjustments = discounts[:events][item.event_id] 
      discounts[:events].delete(item.event_id) 
      popped_from = [:events, item.event_id] 
     end 

     if (adjustment = price_adjustments['$']) 
      adjusted_price = price + adjustment 
     elsif (adjustment = price_adjustments['%']) 
      adjusted_price = price + price * (adjustment/100.0) 
      discounts[popped_from[0]][popped_from[1]] = price_adjustments 
     else 
      adjusted_price = price 
     end 

     adjusted_pricing[item.product_id] = {price: adjusted_price, discount: price - adjusted_price} 

     total += adjusted_price 
    end 

    total += shipping 
end 

メソッドのコードの巨大な作品ですので、私はそれをリファクタリングし、モデルprice_calculatorに移動しようとしています。

def calculate_total_for(order) 
    order_contents = order.current_order_contents 
    product_adjustments = order.product_adjustments 

    shipping = calculate_shipping_price(order_contents, product_adjustments) 

    discounts = construct_discount_hash(product_adjustments) 

    adjusted_pricing = construct_adjusted_price_hash(discounts, order_contents) 

    total_price = adjusted_pricing.inject(0) { |total, (k, v)| total + v[:price] } 

    { 
    total_price: total_price + shipping, 
    shipping: shipping, 
    adjusted_pricing: adjusted_pricing 
    } 
end 

私は、独自のクラスに、以前の巨大なメソッドを移動する過程では、基本的には、まだやったと、そのクラスのようなcalculate_shipping_price、construct_discount_hashに独立したプライベートメソッドにロジックを分割どのような。

私はそれが良いコードから遠いことを知っています。読みやすさの面では私的手法の方がいいですが、テストをするのが難しいと感じ始めます。うまくいけば、ここの誰かがアドバイスやガイドラインを与えることができます。ルビのコードの上でリファクタリングするのに最適な方法は何ですか？

PS：Ruby/Railsの新機能です。以前はほとんどC＃/ Javascriptで記述していましたので、私が慣れていないことをするためのイディオムやルビーの方法があります。

Answer 1

上記の例では、Extract Class from Methodリファクタリングを使用し、モデル内のすべてを移動する代わりにService Objectを使用します。

class Test 
    def total 
    order = @cart.get_or_create_order 
    order_contents = order_contents_for(order) 

    discounts = { 
     events: {}, 
     subjects: {}, 
     products: {} 
    } 

    service = CalculateTotal.new(order, order_contents, discounts) 

    if service.success? 
     # Success logic 
    else 
     flash[:error] = service.error 
     # Failure logic 
    end 
    end 
end 

class CalculateTotal 
    attr_reader :success, :error 

    def initialize(order, order_contents, discounts) 
    @order = order 
    @order_contents = order_contents 
    @discounts = discounts 
    end 

    def call 
    sum_orders + adjusted_prices + shipping 
    end 

    def success? 
    !@error 
    end 

    private 

    def sum_orders 
    # Logic 

    if something_fails 
     @error = 'There was an error calculating the price' 
    end 

    # Logic 
    end 

    def adjusted_prices 
    # Logic 
    end 

    def shipping 
    # Logic 
    end 
end