This gem requires a significant refactor

[ocvit]

Hi,

Few minutes into a code and this is what strikes me:

1. File structure.
   • serpapi/serpapi.rb that hosts just a Client class - should be serpapi/client.rb
   • SerpApi::Client::VERSION that is used as a gem version - should be in a separate file serpapi/version.rb using SerpApi::VERSION namespace
   • serpapi/error.rb does not include corresponding Error module, just SerpApiException class - should be enclosed in unified Errors module, e.g.:

     # serpapi/errors.rb
     module SerpApi
       module Errors
         # all your custom errors
       end
     end

   • specs should represent actual file structure, e.g.:

     spec/serpapi/client_spec.rb    // client config specs
     spec/serpapi/client/*_spec.rb  // all the integration tests that are currently just laying around
     spec/serpapi_spec.rb           // should include VERSION check only
     

2. Don't use open-uri. It creates temporary file for each response, i.e. redundant IO operation that just degrades performance. BTW, why do you have different HTTP clients in different gems? Some uses faraday, some open-uri, some http etc.
3. This:
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Lines 11 to 14 in 0a80c85

   	attr_accessor :timeout
   	# Default parameters provided in the constructor (hash)
   	attr_accessor :params

   
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Lines 32 to 44 in 0a80c85

   	def initialize(params = {})
   	# set default read timeout
   	@timeout = params[:timeout] || params['timeout'] || 120
   	@timeout.freeze
   	# delete this client only configuration keys
   	params.delete('timeout') if params.key? 'timeout'
   	params.delete(:timeout) if params.key? :timeout
   	# set default params safely in memory
   	@params = params.clone || {}
   	@params.freeze
   	end

   
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Lines 95 to 97 in 0a80c85

   	def engine
   	@params['engine'] || @params[:engine]
   	end

   
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Lines 100 to 102 in 0a80c85

   	def api_key
   	@params['api_key'] || @params[:api_key]
   	end

   
   Can be simplified to:

   class SerpApi # should be Client
     DEFAULT_TIMEOUT = 120
   
     attr_accessor :timeout
     attr_reader :params, :engine, :api_key
   
     def initialize(params = {})
       @params = params.transform_keys(&:to_sym)
   
       @timeout = @params[:timeout] || DEFAULT_TIMEOUT
       @engine  = @params[:engine]
       @api_key = @params[:api_key]
     end
   end

4. So which one?
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Line 10 in 0a80c85

   # HTTP timeout in seconds (default: 120)

   
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Line 27 in 0a80c85

   # timeout [Integer] HTTP read max timeout in seconds (default: 60s)

5. No need in nil as third param, it's optional:
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Line 84 in 0a80c85

   get("/searches/#{search_id}.#{format}", format, nil)

6. This:
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Line 113 in 0a80c85

   query = (@params || {}).merge(params || {})

   
   Can be simplified to:

   query = @params.merge(params)
   # @params can't be nil because of default value {} in #initialize
   # params can't be nil because of default value {} in #get

7. This:
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Line 119 in 0a80c85

   query.delete_if { |_, value| value.nil? }

   
   Can be simplified to:

   query.compact!

8. SerpApiException should not be raised for HTTP transport errors, it's misleading just from the naming convention. There should be a separate error class or just a transparent bypass.
   

   serpapi-ruby/lib/serpapi/serpapi.rb

   Lines 138 to 145 in 0a80c85

   	rescue OpenURI::HTTPError => e
   	data = JSON.parse(e.io.read)
   	raise SerpApiException, "error: #{data['error']} from url: #{url}" if data.key?('error')
   	raise SerpApiException, "fail: get url: #{url} response: #{data}"
   	rescue => e
   	raise SerpApiException, "fail: get url: #{url} caused by: #{e}"
   	end

9. No full branch coverage in simplecov, you just ignore bunch of untested places:

   # do this
   SimpleCov.start do
     enable_coverage :branch
   
     add_filter "/spec/"
   end

10. describe/it blocks are just bad. describe should refer to namespaced objects, currently they do context role.
11. No VCRs and dependence on specific API_KEY with high request limit makes testing impossible for other contributors rather than those who work in company.

[jvmvik]

finally I have started implemented all the changes suggested

• 2f6dcbb (1, 2, 3, 4, 7)
• 89cce53 (documentation mostly)
• 222a2ed (5 + 6)

8 to 11 to go.

[jvmvik]

8,9 fix
10. describe / it follows rspec documentation.
11. API_KEY are required to access Serpapi. rake test now runs only a subset of tests which lower the credit usage requirements.