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.

I have an API with the following code

    class API::V1::ReceptionController < API::V1::APIController
      before_filter :ensure_document_exists

      def produto
        jid = ProdutoWorker.perform_async params[:document]
        render json: {jid: jid}, :status => 200
      end

      def cliente
        jid = ClienteFornecedorWorker.perform_async params[:document]
        render json: {jid: jid}, :status => 200
      end

      def nota_fiscal
        jid = NotaFiscalWorker.perform_async params[:document]
        render json: {jid: jid}, :status => 200
      end

      def venda
        jid = VendaWorker.perform_async params[:document]
        render json: {jid: jid}, :status => 200
      end

      def reducaoz
        jid = ReducaoZWorker.perform_async params[:document]
        render json: {jid: jid}, :status => 200
      end

      def impressora_fiscal
        jid = ImpressoraFiscalWorker.perform_async params[:document]
        render json: {jid: jid}, :status => 200
      end
..............

and so on. What happens is: every method call a different worker, but all the body's methods are pretty much the same. My question is: how can I reduce this methods to maybe just one?

share|improve this question
    
Can you show what ProdutoWorker means? As your code is currently missing context. –  skiwi Jul 23 at 12:01
    
yes, of course! params[:document] will always be a JSON that represents a model (a product, a client, an order and so) any of these SomethingWorker receives this JSON and process him following some internal rules –  Luiz K. Jul 23 at 12:05

3 Answers 3

up vote 4 down vote accepted

Some notes:

  • I don't know if there is an agreement on the community on that, but I would definitely use the de-facto language of the computer industry (translating the API routes if needed, of course).

  • I'd write parens on "normal" (non-DSL) method calls.

  • That's a lot of actions. Why don't you refactor it to have a unique action with a param?

I'd write:

class API::V1::ReceptionController < API::V1::APIController
  before_filter :ensure_document_exists
  WorkerClasses = {:produto => ProdutoWorker, ....}

  def process
    worker_class = WorkerClasses[params[:key].to_sym]
    if worker_class
      jid = worker_class.perform_async(params[:document])
      render(json: {jid: jid}, :status => 200)
    else
      render(json: {errors: "Unknown key: #{params[:key]}"}, :status => 400)
    end
  end

(and change the routes accordingly, of course)

share|improve this answer
    
that is EXACTLY what I was thinking! thanks –  Luiz K. Jul 23 at 13:11

I'm not familiar with Ruby, so I will write a no-code answer here, discussing what you could change.

I am assuming that ProdutoWorker, etc. all extend a Worker class, where extending is an OOP-definition here.

Then you should make a method that takes the Worker as input, and then calls something along the lines of:

jid = worker.perform_async params[:document]
render json: {jid: jid}, :status => 200

where worker is the input.

Then you can save both one line per method definition you currently have, and more importantly you abstract away the logic into your helper method, so you cannot make a mistake in the logic of one of your implementations.

share|improve this answer

What I would do is.

before_filter :render_json

def render_json
  render json: {jid: jid}, :status => 200
end

And then remove that line from all your controller actions.

share|improve this answer

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.