5
\$\begingroup\$

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)
                no_of_copies = ((order.answers.where(question_id: 
             [37,15]).length > 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 < 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"
               upload = Invoice.upload(save_path, file_name)
               response['result'] = upload
               response.merge! ApiStatusList::OK
            else
                response.merge! ApiStatusList::INVALID_REQUEST
            end 
            render :json => response 
        end
\$\endgroup\$
5
\$\begingroup\$

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).

\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Wayne Conrad Dec 30 '16 at 22:51
  • 1
    \$\begingroup\$ This type of answer is really what I love about Code Review! \$\endgroup\$ – Marc-Andre Jan 10 '17 at 3:16
4
\$\begingroup\$

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]).length > 0) ? 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 =
  if order.answers.where(question_id: [37,15]).length > 0
    order.answers.where(question_id: [37,15]).first.body
  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 =
  if order.answers.where(question_id: [37,15]).length > 0
    order.answers.where(question_id: [37,15]).first.body
  else
    0
  end.to_i + 1

Prefer this:

order_answer = order.answers.where(question_id: [37, 15]).first
no_of_copies =
  if order_answer
    order_answer.body
  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
no_of_copies = (order_answer ? order_answer.body : 0).to_i + 1

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]
order_answer = order.answers.where(question_id: question_id).first

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]
order_answer = order.answers.where(
  question_id: BEST_QUESTION_EVER_ID
).first

Consider joining the two servers into one.

Why do you have two servers running on the same machine where one calls the other? This code could be greatly simplified if the two servers were joined into one server.

\$\endgroup\$
  • \$\begingroup\$ +1, except the last bit about joining the two into one server. This appears to be part of a Micro Services architecture, where one service specializes in Orders, and the other one for Invoices. A perfectly good separation given the architecture (assuming the architecture is warranted). \$\endgroup\$ – Greg Burghardt Jan 20 '17 at 17:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.