Refactoring Aweful Procedural Controller Logic

[Rajeev Bharshetty]

I have started to see my controllers growing more and more procedural and huge over time.

Here is an example of my controller.

  def create

    uploaded_file = params[:batch_payouts_csv]
    unless valid_file_type?(uploaded_file)
      render json: {"error_message" => I18n.t("merchant.messages.errors.invalid_file_type")}, status: 400
      return
    end

    uploaded_csv_file = Batch::CsvFile.new(uploaded_file.path)
    unless uploaded_csv_file.valid_size?
      render json: {"error_message" => I18n.t("merchant.messages.errors.exceeding_file_size")}, status: 400
      return
    end

    filename_with_timestamp = "#{DateTime.now.strftime("%Q")}_#{uploaded_file.original_filename}"
    merchant_employee = @current_user
    currency = Currency.find(batch_payout_params[:currency_id])
    batch_payout = BatchPayout.create(merchant_employee: merchant_employee, currency: currency)
 
    unless request.xhr?
      response = {filename: filename_with_timestamp, status: "success", location: merchants_batch_payouts_path(batch_payout)}
      render text: response.to_json, status: 201 \
      return
    end
    render json: {"filename" => filename_with_timestamp}, location: merchants_batch_payouts_path(batch_payout), status: 201
  end

As you can guess, my controller action has started to grow in size and needs a serious effort at refactoring.

The initial approach which I followed was to plug in the guard checks into a before_filter. Is it a good approach to refactor the guard checks into private methods and plug them into before_filters ?

Need help in identifying, If there is a more OOP way of extracting the business logic into a class and delegating the render and redirecting to the controller.

Thanks and Regards

Rajeev N B

[Amos King]

Rajeev,

You have 4 calls to render. Is there a way that you can make that one? Perhaps saving the hashes that are passed to render into a variable, or even making an object out of them. It could be called Response. I'm not sure of what behavior you would like in them at this point, but thinking in that direction might help. Could the uploaded file size limit be handled by your web server? I know there are configurations for that in nginx, and apache. This would reduce the work that even makes it to your application.

Your first :unless: has a method "valid_file_type", could you instead place that on the file itself? "uploaded_file.valid_file_type?"

Could those validations live inside of the file type? Should the controller care about the file type or the size?

filename_with_timestamp might be better as a method hanging off of the file also. uploaded_file.name_with_timestamp

If this is the only place that the name_with_timestamp is used you might think about a decorator. uploaded_file = TimestampedFile.new(uploaded_file); uploaded_file.name.

'

Ok, so I've rambled out quite a few ideas. They probably aren't laid out in a coherent manor. Let me know if you have any other questions.

Amos King

"The bitterness of poor quality remains

long after the sweetness of low price is forgotten" - Benjamin Franklin

http://dirtyInformation.com
http://github.com/Adkron

http://thisagilelife.com # podcast

=======================================================
I welcome VSRE emails. Learn more at http://vsre.info/
=======================================================

--
You received this message because you are subscribed to the Google Groups "Objects on Rails" group.
To unsubscribe from this group and stop receiving emails from it, send an email to objects-on-rai...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Nicholas Henry]

One option would be to extract the creation of a batch payout into a service with callbacks to a listener (controller):

class BatchPayoutController < ApplicationController
  def create
    CreatingBatchPayout.new(self).call(@current_user, params)
  end
 
  private
 
  def determined_invalid_file_type

    render json: {"error_message" => I18n.t("merchant.messages.errors.invalid_file_type")}, status: 400

  end
 
  def determined_invalid_file_size

    render json: {"error_message" => I18n.t("merchant.messages.errors.exceeding_file_size")}, status: 400   

  end
 
  def batch_payout_created_successfully(batch_payout, file)
    if request.xhr? 
      render json: {"filename" => file.filename}, location: merchants_batch_payouts_path(batch_payout), status: 201    
    else
      response = {filename: file.filename, status: "success", location: merchants_batch_payouts_path(batch_payout)} 

      render text: response.to_json, status: 201

    end 
  end
end
 
class TimeStampedFile < SimpleDelegator
  def filename
    "#{DateTime.now.strftime("%Q")}_#{original_filename}"
  end
end
 
class UploadedFile < SimpleDelegator
  def valid_type?
    # add logic here
  end
end
  
class CreatingBatchPayout
  def initialize(listener)
    @listener = listener
  end
  
  def call(merchant_employee, params)
    uploaded_file = TimeStampedFile.new(UploadedFile.new(params[:batch_payouts_csv]))
    @listener.determined_invalid_file_type and return unless uploaded_file.valid_type?
    
    uploaded_csv_file = Batch::CsvFile.new(uploaded_file.path) 
    @listener.determined_invalid_file_size and return unless uploaded_csv_file.valid_size? 
 
    currency = Currency.find(params[:batch_payout][:currency_id]) 
    batch_payout = BatchPayout.create!(merchant_employee: merchant_employee, currency: currency)
    
    @listener.batch_payout_created_successfully(batch_payout, uploaded_file) 
  end
end

[Nicholas Henry]

Just once correction there, the methods in the BatchPayoutController should not be private.