patternrubyrailsMinor
Rails API for communication between two Rails app where the former app posts data to the latter and the latter return a response
Viewed 0 times
theappreturnresponsecommunicationwhereformerdatarailstwo
Problem
The Controller code for the first app to post data to another app
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 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
endThe 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
endSolution
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:
(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
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
Let's delve into the actual method now.
The first thing you are doing is initializing a couple of variables.
You should remove
The
You can also extract the scope into
..and use..
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
(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
endIt 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])
endYou 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])
endNow 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 predicaCode Snippets
module Api
module V1
class OrderApiController < ApiController
end
end
enddef 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
enddef order
@order ||= Order.includes(:status, :user, payment: [:status])
.find_by(txnid: params[:txnid])
endclass Order < ActiveRecord::Base
scope :eager, -> { includes(:status, :user, payment: [:status]) }
enddef order
@order ||= Order.eager.find_by(txnid: params[:txnid])
endContext
StackExchange Code Review Q#150684, answer score: 5
Revisions (0)
No revisions yet.