ruby-on-rails - 如何以更好的方式修复此代码异味?
问题描述
您将如何识别和修复带有长重复方法的以下代码异味?这是下面的代码气味:
代码气味
class TransactionProcessingService
def initialize(user, product_id)
@user = user
@product = Actions::Base.find_by id: product_id
end
def call
return false unless valid?
ActiveRecord::Base.transaction do
@reservation = @user.reservations.
where(action_id: @product.id).
where("status IS NOT NULL AND status NOT IN ('archived', 'cancelled')").
first_or_initialize
@reservation.update(quantity: (@reservation.quantity.presence || 0) + 1) # Nil guard
update_state!
end
end
protected
def valid?
@product.allowed_for?(@user) and @user.balance >= @product.price
end
def update_state!
if @product.is_a?(Actions::Target)
@user.transactions.create(status: 'sold_target', transaction_type: :product_purchase, amount: @product.price, product: @product)
@user.update_columns(balance: @user.balance - @product.price - @product.discount_for_user(@user))
elsif @product.is_a?(Actions::Lease)
@user.transactions.create(status: 'sold_lease', transaction_type: :product_purchase, amount: @product.price, product: @product)
@user.update_columns balance: @user.balance - @product.price
end
end
end
但是,我尝试对此进行重构,但我觉得我让它变得更糟了。您认为我应该如何重构上面的代码?这是我在下面所做的:
这就是我所做的......
class TransactionProcessingService
def initialize(user, product_id)
@user = user
@product = Actions::Base.find_by id: product_id
end
def call
false unless valid?
ActiveRecord::Base.transaction do
update_user_reservations
create_state
update_balance
end
end
protected
def valid?
@product.allowed_for?(@user) && (@user.balance >= @product.price)
end
def wrapper_around(*)
@product.is_a?
end
def update_user_reservations
result = @user.reservations
.where(action_id: @product.id)
.where("status IS NOT NULL AND status NOT IN ('archived', 'cancelled')").first_or_initialize
result.update(quantity: (quantity.presence || 0) + 1)
end
def update_balance
value = @user.balance - @product.price
target = wrapper_around(Actions::Target)
lease = wrapper_around(Actions::Lease)
@user.update_columns(if target
{ balance: value - @product.discount_for_user(@user) }
elsif lease
{ balance: value }
end)
end
def create_state
product_price = @product.price
target = wrapper_around(Actions::Target)
lease = wrapper_around(Actions::Lease)
@user.transactions.create(if target
{
status: 'sold_target',
transaction_type: :product_purchase,
amount: product_price,
product: product_params
}
elsif lease
{
status: 'sold_lease',
transaction_type: :product_purchase,
amount: product_price,
product: product_params
}
end)
end
end
解决方案
只是为了给您一个想法,您可以将代码中可以识别的不同操作与其他一些类分开:
事务处理:
class TransactionProcessing
def initialize(product_id:, user:)
@user = user
@product_id = product_id
end
def call
return unless product
return unless valid?
ActiveRecord::Base.transaction do
UpdateReservation.new(product: product, user: user).update
update_state!
end
end
private
attr_reader :product_id, :user
delegate :id, :price, to: :product
delegate :quantity, to: :reservation
delegate :balance, :reservations, :transactions, to: :user
def product
Actions::Base.find_by(id: product_id)
end
def update_state!
Object.const_get("#{product.class}Transaction").new(product: product, user: user).update
end
def valid?
product.allowed_for?(user) && balance >= price
end
end
更新预订:
class UpdateReservation
def initialize(user:, product:)
@product = product
@user = user
end
def update
reservation.update(quantity: reservation_quantity)
end
private
attr_reader :user
delegate :id, to: :product
delegate :reservations, to: :user
delegate :quantity, to: :reservation
def reservation
reservations.where(action_id: id).where.not(status: nil)
where('status NOT IN ("archived", "cancelled")').first_or_initialize
end
def reservation_quantity
(quantity.presence || 0) + 1 # Just if quantity.presence can be nil. Otherwise add a rescue.
end
end
动作模块:
module Actions
class CustomTransaction
attr_reader :product, :user
delegate :price, to: :product
delegate :balance, :transactions, to: :user
def initialize(product:, user:)
@product = product
@user = user
end
def update
create_transaction
update_balance
end
def create_transaction
transactions.create(params)
end
def update_balance
user.update(balance: balance_value)
end
private
def params
{ transaction_type: :product_purchase, amount: price, product: product }
end
end
class LeaseTransaction < CustomTransaction
STATUS = 'sold_lease'.freeze
private_constant :STATUS
private
def params
super.merge(status: STATUS)
end
def balance_value
balance - price - discount_for_user
end
def discount_for_user
product.discount_for_user(user)
end
end
class TargetTransaction < CustomTransaction
STATUS = 'sold_target'.freeze
private_constant :STATUS
private
def params
super.merge(status: STATUS)
end
def balance_value
balance - price
end
end
end
在那里你可以看到:
- 位于服务命名空间或文件夹下的类已经是服务,无需使其冗余。
- 在初始化程序中使用
product_id
,避免添加更多代码。product
属于单独的私有方法。 - 给你的班级尽可能少的费用。
- 尽可能将方法委托给您的访问者和其他人。
- 如果存在共享行为,则始终有可能使用继承。
- 给你的方法尽可能少的费用。
is_a?
如果可以,请避免。只要打电话。
即使这需要改进,所以,任何人,请随意。
推荐阅读
- reactjs - TypeError:预期的字符串,但收到未定义的字符串
- r - 使用 purrr 地图打印 ggplot
- sql - 如何编写oracle sql查找特殊字符
- scala - Spark GroupBy 操作性能提升
- reactjs - 以动态百分比从下到上填充圆形动画
- c# - .Net ComException 调用 UserPrincipal.FindByIdentity (0x80005000)
- javascript - 如何使用 v-if 检查 .vue 中的 vue 对象是否为空
- python - 编写修改列表的函数的 Pythonic 方式?
- php - 仅计算包含帖子摘录的帖子
- c++ - 单击 Push 按钮应使用 Qt C++ 在不同窗口中显示绘图