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

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

 module Api
module V1
class OrderApiController < ApiController
def send_invoice_data
response = Hash.new
result = Hash.new
debugger
order = Order.includes(:status, :user, payment:
[:status]).where(txnid: params[:txnid]).first
if order
if (['Notary', 'Attestation','Franking'].include?
order.service.name)
[37,15]).first.body : 0).to_i
else
[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 < ApiController

def order_invoice
response = Hash.new
result = Hash.new
debugger
if params[:order] && params[:order][:txnid]
@order = params[:order]
@order['stamp_amount'] = params[:order][:stamp_amount] || ''
@order['txnid'] = params[:order][:txnid]
@order['service_name'] = params[:order][:service_name] || ''
@order['payment_date'] = params[:order][:payment_date]
@order['payment_mode'] = params[:order][:payment_mode] || ''
:
:
@order['discount'] =  params[:order][:discount] || ''
@no_of_copies = params[:order][:no_of_copies]
pdf = WickedPdf.new.pdf_from_string(
render_to_string(template:
'invoices/generate_invoice.pdf.erb', filename: params[:order]
[:txnid] + ".pdf" ,
type: 'application/pdf', disposition:
'attachment', print_media_type: true))
save_path = Rails.root.join('pdfs', @order['txnid'] + ".pdf")
File.open(save_path, 'wb') do |file|
file << pdf
filename = @order['txnid'] + ".pdf"
end
file_name =  @order['txnid'] + ".pdf"
response.merge! ApiStatusList::OK
else
response.merge! ApiStatusList::INVALID_REQUEST
end
render :json => response
end


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 < ActiveRecord::Base
scope :eager, -> { 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 predicate method:

class Order
def service_i_want?
services.include?(service.name)
end

def services
%w(Notary Attestation Franking)
end
end


This means your send_invoice_data will now look like:

def send_invoice_data
if order
if order.services_i_want?
no_of_copies = # etc
else
no_of_copies = # etc
end

# HTTParty.post etc
else
..
end
end


Again, lots of extraction going on. Either into methods, or other classes.

Looking at the above we can see yet another opportunity for extraction - the no_of_copies. Instead of initializing a variable, extract it into it's own method..

def no_of_copies
if order.services_i_want?
[37,15]).first.body : 0).to_i
else
[37,15]).first.body : 0).to_i + 1
end
end


I think you know where I'm going next ;) Extracting this into its own method has clearly shown us these two branches have essentially the same code. In the effort to DRY things up we should extract further.

First, let's breakdown what you are doing here.

If you have answers that have the question id of 37 or 15, then fetch body of the first answer. If no answers, return 0. Convert result to_i.

To better express this, you should, guess.. extract it into smaller parts! The logic itself probably belongs in the Order class. So first thing would be to extract it there:

class Order
def number_of_copies
# etc
end
end


Because you are now in the Order context, you can better extract the other parts. We want answers with question id of 37 or 15 first..

def copied_answers
end


We can now reduce our number_of_copies method to this:

def number_of_copies
end


MUCH easier to think about! Looking at your logic from this viewpoint, we can now easily rewrite it to give us what we want in a much more elegant way:

def number_of_copies
else
0
end
end


Or if you like your ternaries:

def number_of_copies
end


Let's go back to our copied_answers method for a second.

def copied_answers
end


As the other answer mentioned, you should refrain from using 'magic numbers'. 37 and 15 above are magic because they give me no idea of what they represent. Extract them into methods or constants or whatever. Just hide them behind something descriptive so I can use this:

def copied_answers
# or
end


Describe what the questions are so somebody can understand why these question ids are connected to 'copied' answers.

We also have another piece of logic to address. If the order service name is not in that services list above (services_i_want), then we increment the number of copies. I'm open to suggestions, but my first attempt at that would be:

def number_of_copies
copies = 0

end

copies += 1 if services_i_want?

copies
end


That can no doubt be improved, but I'm writing that just as a way to match the current logic. It is, regardless, a vast improvement of what we had previously.

Let's return to your send_invoice_data method.

Because we have extracted the logic that determines the number of copies into number_of_copies on the Order itself, our send_invoice_data now looks like:

def send_invoice_data
if order
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" => order.number_of_copies}}.to_json,
:headers => { 'Content-Type' => 'application/json' }
}
)
else
response = ApiStatusList::INVALID_REQUEST
end

render json: response
end


Again, much easier to see what the hell is going on here now. If we have an order, we send that order to generate invoice endpoint and return the response. If no order found, we return INVALID_REQUEST.

Let's first extract the post into it's own method..

def generate_invoice
# HTTParty.post(etc)
# etc
# etc
end


Notice this method from before? The natural progression would be to first extract into a method, then as that requires further breaking down, extract into other methods. If those methods begin to pollute the class we are in, extract it into its own object.

The end result is this:

module Api
module V1
class OrderApiController < ApiController
def send_invoice_data
if order
render json: generate_invoice
else
render json: ApiStatus::INVALID_REQUEST
end
end

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

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


I will stop extracting things at this point and allow you to work through the rest by yourself. First place to start would be to create your GenerateInvoice class and throw all your logic into there. After that, just continue to extract things into small, descriptive methods, and, if necessary, other objects.

Use the advice in these answers and apply what you've learned to your second file. It's the same principles and if you follow them will lead to cleaner code.

A starting point might be the payment_date value on your post request body. Extract into Order and simply call order.payment_date.

I hope this has helped you somewhat. If you want more help, please follow the advice above and refactor your code, then post another question and I will give feedback there (do not update this question as will render my answer useless to other people).

• I gave answers, but you gave more than just answers. I like your examples and how you show progressive refactoring improving the code. Bravo, nicely done. – Wayne Conrad Dec 30 '16 at 22:51
• This type of answer is really what I love about Code Review! – Marc-Andre Jan 10 '17 at 3:16

# Code formatting

The code shown in the question sometimes uses two spaces, sometimes four. Prefer two spaces for indentation; this is a a universal Ruby standard.

# Business logic in the controller

Try to remove business logic from the controllers and place it instead in service objects. The controller should be just glue that provides the HTTP part of the application.

# Replace large expressions of the trinary operator if...else...end

If an expression using the trinary operator is long:

no_of_copies = ((order.answers.where(question_id:
[37,15]).first.body : 0).to_i + 1


Then express it instead using if...else...end. In Ruby, an if statement's value is the value of the last expression in the block that was executed, so instead you can do this:

no_of_copies =
else
0
end.to_i + 1


There are other issues with this bit of code; I'll address those.

# Use more temporaries

The code I rewrote above still needs improvement. Here, we see the where clause is repeated. By using a temporary, the code can be improved. So, instead of this:

no_of_copies =
else
0
end.to_i + 1


Prefer this:

order_answer = order.answers.where(question_id: [37, 15]).first
no_of_copies =
else
0
end.to_i + 1


With the use of the temporary, the condition is now short enough that you could go back to using the trinary operator, if you like:

order_answer = order.answers.where(question_id: [37, 15]).first


# Don't repeat yourself (DRY)

Each idea--such as the search key--should be expressed just once. For example, the question ID [37, 15] is expressed repeatedly. Instead, express it just once:

question_id = [37, 15]


# Assign meaning to magic numbers.

The question Id [37, 15] is a "magic number." We call it "magic" because it has no obvious meaning to the reader. What is special about that question? Only you know, but by creating a constant, you can give it meaning. For example, if it was the ID of the best question ever, then:

BEST_QUESTION_EVER_ID = [37, 15]