1
\$\begingroup\$

I think the snippet is too nested having too many blocks. It needs to be refactored to make the logic more readable.

  • The try exception is the first level block
  • Timeout::timeout is the next level block
  • Some loop-like blocks inside it

How can I make it more readable?

  def https_request(server_url, header, content)
    begin
      Timeout::timeout(@@TIMEOUT_PERIOD) do
        hack_ssl_options
        uri = URI.parse(server_url)
        https = Net::HTTP.new(uri.host,uri.port)
        https.use_ssl = true
        options_mask = OpenSSL::SSL::OP_NO_SSLv2 + OpenSSL::SSL::OP_NO_SSLv3 + OpenSSL::SSL::OP_NO_COMPRESSION
        https.ssl_options = options_mask
        https.verify_mode = OpenSSL::SSL::VERIFY_NONE
        req = Net::HTTP::Post.new(uri.path, initheader = header)
        req.body = content
        headers = req.each_header.inject({}) { |h,(key, value)| h[key] = value ; h }
        resp = https.request(req)
        response_headers = resp.each_header.inject({}) { |h,(key, value)| h[key] = value ; h }
        return { code: resp.code,
                 header: response_headers,
                 body: resp.body }
      end
    rescue Timeout::Error
      return { code: @@timeout_error_code,
               header: nil,
               body: @@timeout_error_msg }
    rescue Exception => e
      return { code: @@unknown_error_code,
               header: nil,
               body: @@unknown_error_msg }
    end
  end
\$\endgroup\$
2
\$\begingroup\$

to answer your questions first:

  • you don't need begin/end if it covers whole method, you can rescue directly from method,
  • Net::HTTP already handles timeout, no need to build custom wrapper around it,

now there are few other things that should be addressed:

  • inline things when possible
  • don't use @@ ever, use constants if you need to
  • don't rescue from Exception, rescue from StandardError instead (or nothing as ruby defaults to StandardError anyways)
  • try to be consistent with returned result, i.e. header should always return hash, never nil. Otherwise you will need to add conditions in places where you handle code
  • OpenSSL::SSL::VERIFY_NONE this is bad, you want to verify certificate.
  • do not use Net::HTTP, there are much more rubout gems available, i.e. Typhoeus

Finally it could be rewritten to something like this:

def https_request(server_url, header, content)
  hack_ssl_options
  uri = URI.parse(server_url)

  connection = Net::HTTP.new(uri.host, uri.port)
  connection.use_ssl = true
  connection.ssl_options = OpenSSL::SSL::OP_NO_SSLv2 + OpenSSL::SSL::OP_NO_SSLv3 + OpenSSL::SSL::OP_NO_COMPRESSION
  connection.verify_mode = OpenSSL::SSL::VERIFY_NONE
  connection.read_timeout = TIMEOUT_PERIOD

  request = Net::HTTP::Post.new(uri.path, initheader = header)
  request.body = content

  response = connection.request(req)

  { code: response.code, header: response.to_hash, body: response.body }
rescue Net::ReadTimeout => e  
  { code: CODE_TIMEOUT, header: {}, body: e.message }
rescue => e
  { code: CODE_UNKNOWN, header: {}, body: e.message }
end

This code works slightly different from your original version, you may need to adjust it.

\$\endgroup\$

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.