I have a rails app ported from php codebase. I have a long controller method that basically calculate a total price based on the items in my cart. This is a legacy method that is directly ported from the php code.
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
Above code is a huge pieces of code for a method, so I'm trying to refactor it and move it to a model 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
What I did basically, still in the process of move the previous huge method into its own class and split the logic into a separate private method in that class such as calculate_shipping_price, construct_discount_hash.
I know it's far from a good code. Break it into private method ooks good in term of readability, but I start to feel it's getting hard to make a test for it. Hopefully, someone here can give an advice or a guideline, what's the best way to refactor above code in ruby.
PS: I'm new to ruby/rails, I previously code mostly in C#/Javascript, so there are some idiom or ruby-way to doing things that I'm not familiar with.
For the example you mentioned, I would use Extract Class from Method refactoring and use a Service Object instead of moving everything in a model.
Here is an overview of how to do it, of course I leave the implementation for you:
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
Collected from the Internet
Please contact [email protected] to delete if infringement.
Comments