current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am working on a payment process. Upon a successful payment process, I have to create multiple records. For that, I have made this class:

class DatabaseMaintainer

  def initialize(merchant, card, payment_method)
    @merchant = merchant
    @card = card
    @payment_method = payment_method
  end

  def maintain_records(customer, payment_details, response)
    rate = RateCatalog.get_rate(@merchant.id, @card.id, @payment_method.id)
    amount =  payment_details['total'].to_d
    commission = rate.rate *  amount / 100
    payable = amount - commission
    customer.organization_id = @merchant.id
    customer.save

    cash_transaction = CashTransaction.create!(
        amount: amount,
        receipt_id: response.authorization,
        rate_catalog_id: rate.id,
        customer_id: customer.id,
        redirect_url: payment_details['redirect_url']
    )

    Settlement.create!(
        commission_amount: commission,
        payable_amount: payable,
        cash_transaction_id: cash_transaction.id
    )

    invoice = Invoice.create!(
                    invoice_number: "#{CommonConstants::INVOICE_NUMBER_PREFIX}#{@merchant.abbreviation}_#{payment_details['invoice_id']}",
                    total: amount,
                    status: Invoice.statuses[:paid],
                    organization_id: @merchant.id,
                    cash_transaction_id: cash_transaction.id,
                    description:  payment_details['description']
    )

    item_details = payment_details['items']
    item_details.each do  |x|
      x[:bill_id] = invoice.id
      x[:bill_type] = 'Invoice'
    end 

    PurchasedGood.create!(item_details)
  end
end

And I am using it as follows:

DatabaseMaintainer.new(@merchant, card_brand, payment_method).maintain_records(@customer, invoice_details, gateway_reply[:message])

Now, even if the code works fine, it clearly is not an optimal design.

I am looking for suggestions to refactor it. Also, is having a class for creating DB records solely justified in my scenario?

share|improve this question

2 Answers 2

up vote 0 down vote

Put that business logic away!

First thing you should do is look at the code and try to find business logic which belongs inside your models.

This looks like something which your models should be handling:

rate = RateCatalog.get_rate(@merchant.id, @card.id, @payment_method.id)
amount =  payment_details['total'].to_d
commission = rate.rate *  amount / 100
payable = amount - commission

This is another:

invoice_number: "#{CommonConstants::INVOICE_NUMBER_PREFIX}#{@merchant.abbreviation}_#{payment_details['invoice_id']}"

Classes should do one job, and do it well.

is having a class for creating DB records solely justified in my scenario?

Its more than justified. The Single Responsibility Principle states that every class should have responsibility over a single part of the functionality provided by the software.

So moving this extremely complicated logic into its own class is a good first step. But don't stop there!

In fact when you have classes with extremely generic names like DatabaseMaintainer which not tell you right away what its role in the software is you have a code smell. The same when you have a method like maintain_records. It tells us nothing about what it actually does or what the expected output is.

Transactions

Database translactions can be used to make the setup stateless. If for example the CashTransaction.create! is successful but Invoice.create! fails we may not want a lone CashTransaction to be created. Using a transaction would let us roll back the changes.

Service objects to the rescue

An established pattern which I find really useful in real world use cases are Service Objects. The idea is to simplify complex process by creating plain old ruby classes which perform a single job.

As this answer is already really long i'm not going to go into detail but look at this monster method and try to isolate some distinct steps and think of them in terms of input and output.

Afterword.

Sorry if this is not more hands on - I read though your code a few times and since I don't know exactly the circumstances (is this called after some kind of payment gateway? or some kind of manual step which is done by a clerk after receiving payment?) I just got lost. In general I think you should look a bit at your domain modeling and try to get your models to work better together so that the instantiation parameters are simpler.

share|improve this answer
up vote 1 down vote

My Suggestions are

  • If this class is not going to be instantiated you can remove the initialize method and even make it a module
  • Now you donot need to use instance variables , local variable passed via params work here
    According to the book 'Object Oriented Design in Ruby - By Sandi Metz' a method need not know the order of params and even the ClassNames it is going to create objects from. Rather you can pass Hash of parameters and you can use like

Advantage: You can pass params in any order

def maintain_records(param)  
  # set fallback/default params
  param.merge(default_params)
  customer, payment_details, response = param[:customer], param[:payment_details], param[:response]
share|improve this answer
    
You are right about using a module if you do not plan to create instances but I'm not sure if this even is an improvement. Now you still have a monster method but with a crazy amount of parameters masked into a hash. At least before he would get a error if called with the wrong params. –  papirtiger Aug 5 at 20:17

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.