-5
\$\begingroup\$

I have coded this Ruby code. Everything here works perfect as intended, however it is a real mess. What would be the best way to clean this up with best practices and such?

  class new_class
    attr_reader :shop_id
    SECRET_API_KEY = "asdasdasQWRESFSDFVSDFASDFSADFASDF123123ASDASD$%$%"

      def set_shop_id(shop_Id)
           @shop_id = shop_Id
      end

      def orders
         params = Hash.new
         params[:basic_auth] = {username: SECRET_API_KEY, password: ''}
         orders_json = some_http_library.get("xyz/orders", params)
         return JSON.parse(orders_json)
     end

    def products
        params = Hash.new
        params[:basic_auth] = {username: SECRET_API_KEY, password: ''}
        products_json = some_http_library.get("xyz/products", params)
        return JSON.parse(products_json)
    end

    def product(id)
        params = Hash.new
        params[:basic_auth] = {username: SECRET_API_KEY, password: ''}
        product_json = some_http_library.get("xyz/products/#{id}", params)
        return JSON.parse(product_json)
    end
end
\$\endgroup\$
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$ – Jamal Jan 21 '16 at 21:35
  • \$\begingroup\$ I wouldn't store your API key in a class. Use environment variables. \$\endgroup\$ – Dan Pantry Jan 21 '16 at 21:50
  • \$\begingroup\$ You should not repost an identical copy of a question that has been closed, nor should you create a second account to do it. \$\endgroup\$ – 200_success Jan 21 '16 at 22:01
1
\$\begingroup\$
  1. Returns aren't necessary in Ruby. The last line gets returned out of methods.

  2. You create the same has every time. The creation of that Hash could be a private method itself. If it only needs to be created once and will not change during the lifetime of this class' lifecycle, then you can create it upfront in a constructor or just store it in a instance variable.

If I had to rewrite it, it would look more like this:

class NewClass
  SECRET_API_KEY = "asdasdasQWRESFSDFVSDFASDFSADFASDF123123ASDASD$%$%"
  attr_reader :shop_id

  def initialize(shop_id)
    @shop_id = shop_id
    @params = auth_params
  end

  def orders
    retrieve_resource_from_url "xyz/orders"
  end

  def products
    retrieve_resource_from_url "xyz/products"
  end

  def product(id)
    retrieve_resource_from_url "xyz/products/#{id}"
  end

  private

  def retrieve_resource_from_url(url)
    response = some_http_library.get(url, @params)
    JSON.parse response
  end

  def auth_params
    {
      basic_auth: {
        username: SECRET_API_KEY,
        password: ''
      }
    }
  end
end
\$\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.