HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Rails API for communication between two Rails app where the former app posts data to the latter and the latter return a response

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theappreturnresponsecommunicationwhereformerdatarailstwo

Problem

The Controller code for the first app to post data to another app

module Api
 module V1
 class OrderApiController  0) ? order.answers.where(question_id: 
            [37,15]).first.body : 0).to_i 
            else
                no_of_copies = ((order.answers.where(question_id: 
                [37,15]).length > 0) ? order.answers.where(question_id: 
                [37,15]).first.body : 0).to_i + 1
            end
             response.merge! ApiStatusList::OK
             response = 
                 HTTParty.post('http://localhost:3001/api/v0/generate_invoice?
                 key=docket', :body => { "order" => {
                      "id" => order.id, 
                      "txnid" => order.txnid, 
                      "service_name" => order.service.name, 
                      "payment_date" => order.payment ?  
                       order.payment.created_at.strftime('%d/%m/%Y') : 
                       order.created_at.strftime('%d/%m/%Y'), 
                                :
                                :
                      "delivery_amount" => order.delivery_amount || '',
                      "no_of_copies" => no_of_copies}}.to_json, :headers => { 
                     'Content-Type' => 'application/json'})
        else
            response.merge! ApiStatusList::INVALID_REQUEST
        end
        render :json => response
    end 
   end
   end
   end


The second controller api code accepts data from the former app, creates and generates the invoice pdf using wicked_pdf, uploads it to aws and returns the link of the stored pdf as response to latter app

module Api
module V0
    class InvoiceApiController  response 
        end

Solution

I will try to approach this from top to bottom, and mention things as I notice them.

(1) First obvious one is indentation, as mentioned in the other answer. The standard is 2 spaces. Nested modules and classes should be indented:

module Api
  module V1
    class OrderApiController < ApiController
    end
  end
end


(2) Extract logic into object(s) or other methods. You have started what is and will increasingly be a difficult piece of code to maintain. If you find yourself dumping everything into a single method, and that method grows beyond 4 or 5 lines, you could probably clean it up by using multiple methods and/or objects.

That's all well and good, but what does this look like? This means your send_invoice_data should look something like this:

def send_invoice_data                                                           
  render json: generate_invoice                                      
end                                                                             

def generate_invoice                                                            
  @generate_invoice ||= Api::GenerateInvoice.post(order)                        
end                                                                             

module Api                                                                      
  class GenerateInvoice                                                         
    def initialize(order)                                                       
      @order = order                                                            
    end                                                                         

    def self.post(order)                                                        
      new(order).post                                                           
    end                                                                         

    def post                                                                    
      HTTPParty.post(url, body: data)                                           
    end                                                                    

    private                                                                     

    def url                                                                     
      'http://localhost:3001/api/v0/generate_invoice?key=docket'                
    end                                                                         

    def data                                                                    
      {                                                                         
        id: @order.id,                                                          
        txnid: 'etc'                                                            
      }                                                                         
    end                                                                         
  end                                                                           
end


It is important you understand what we have done above, and the benefits that gives you. For one, it has instantly cleaned up the controller method and now you can solely worry about dealing with the response for the action.

The GenerateInvoice object has allowed us to not just extract all of that messy logic, but given us the ability to further break it down into small, descriptive methods. This will greatly improve the readability and maintainability of it. You also have the added benefit of being able to use this class elsewhere, should you need to.

Let's delve into the actual method now.

The first thing you are doing is initializing a couple of variables. response is either being assigned later (response = HTTParty) or you are using merge (response.merge! etc). You can remove this variable initialization and simply assign it later. Remove your merge and use response = ApiStatusList::INVALID_REQUEST.

You should remove debugger before you commit the code.

The Order.includes part should be extracted into a method:

def order
  @order ||= Order.includes(:status, :user, payment: [:status])
                  .find_by(txnid: params[:txnid])
end


You can also extract the scope into Order itself:

class Order  { includes(:status, :user, payment: [:status]) }
end


..and use..

def order
  @order ||= Order.eager.find_by(txnid: params[:txnid])
end


Now this might appear to be a bit extreme or contrived, but it is important to notice what I'm doing as I refactor this code. What is informing my main design decisions is the idea of extracting things into smaller, descriptive pieces. Just following that path alone is vastly improving the design of the code. Keep that in mind as you write and refactor code.

Another great place for extraction is checking whether and order is of a certain service type. You could extract this directly into your Order class again under what we call a predica

Code Snippets

module Api
  module V1
    class OrderApiController < ApiController
    end
  end
end
def send_invoice_data                                                           
  render json: generate_invoice                                      
end                                                                             

def generate_invoice                                                            
  @generate_invoice ||= Api::GenerateInvoice.post(order)                        
end                                                                             

module Api                                                                      
  class GenerateInvoice                                                         
    def initialize(order)                                                       
      @order = order                                                            
    end                                                                         

    def self.post(order)                                                        
      new(order).post                                                           
    end                                                                         

    def post                                                                    
      HTTPParty.post(url, body: data)                                           
    end                                                                    

    private                                                                     

    def url                                                                     
      'http://localhost:3001/api/v0/generate_invoice?key=docket'                
    end                                                                         

    def data                                                                    
      {                                                                         
        id: @order.id,                                                          
        txnid: 'etc'                                                            
      }                                                                         
    end                                                                         
  end                                                                           
end
def order
  @order ||= Order.includes(:status, :user, payment: [:status])
                  .find_by(txnid: params[:txnid])
end
class Order < ActiveRecord::Base
  scope :eager, -> { includes(:status, :user, payment: [:status]) }
end
def order
  @order ||= Order.eager.find_by(txnid: params[:txnid])
end

Context

StackExchange Code Review Q#150684, answer score: 5

Revisions (0)

No revisions yet.