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?
((order.answers.where(question_id:
[37,15]).length > 0) ? order.answers.where(question_id:
[37,15]).first.body : 0).to_i
else
((order.answers.where(question_id:
[37,15]).length > 0) ? order.answers.where(question_id:
[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
answers.where(question_id: [37, 15])
end
We can now reduce our number_of_copies method to this:
def number_of_copies
((copied_answers.length > 0) ? copied_answers.first.body : 0).to_i
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
if copied_answers.any?
copied_answers.first.body.to_i
else
0
end
end
Or if you like your ternaries:
def number_of_copies
copied_answers.any? ? copied_answers.first.body.to_i : 0
end
Let's go back to our copied_answers method for a second.
def copied_answers
answers.where(question_id: [37, 15])
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
answers.where(question_id: [MY_QUESTION, ANOTHER_QUESTION])
# or
answers.where(question_id: question_ids)
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
if copied_answers.any?
copies = copied.answers.first.body.to_i
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).
