5
\$\begingroup\$

The code takes in a hash that comes from a webform and also works with product data from the database. It then uses an eBay gem I built that connects with the eBay API and then returns a response object that you can call #extract on to return desired data. #extract takes an optional argument that will return data other than the default.

I've been working with this code for quite some time, trying to figure out how I can break it up. It feels very clunky and unorganized.

require 'ebay'

class ListingForm
  include ActiveModel::Model

  attr_accessor :title, :price, :subtitle, :product_id, :duration

  def initialize(form={})
    @title = form[:title]
    @price = form[:price]
    @subtitle = form[:subtitle]
    @product_id = form[:product_id]
    @duration = form[:duration]
    @product = set_product if @product_id
  end

  def post
    yield self if block_given?
    ebay_response = api.add_item(item: item)
    item_id = ebay_response.extract
    if valid_listing?(item_id)
      Listing.new(product_id: @product.id, 
                  item_number: item_id).save
    else
      raise "invalid item number"
    end
    ebay_response
  end

  def valid_listing?(item_id)
    item_id.to_i != 0
  end

  def item_specifics
    item_specifics = []
    @product.properties.each do |k,v|
      name = k.to_s.for_humans
      value = "#{v}#{PROPERTY_FORMATTER[k.to_sym]}"
      if value == "1"   
          value = "included" 
      elsif value == "0"
        next
      end 
      item_specifics << Ebay::Types::NameValueList.new({name: name, value: value})  
    end
    item_specifics
  end

  def remote_images
    remote_images = [] 
    @product.images.each do |i|
      url = "http://www.howmuchcomputer.com" + i 
      remote_url = @api.upload_site_hosted_pictures({:external_picture_url => url}) 
      remote_images << remote_url.extract(ignore_errors: true) 
    end 
    remote_images.reverse!
  end

  def template(path)
    ERB.new(File.open(path, "r").read).result(binding).to_s
  end

  def item
    Ebay::Types::Item.new({
      :start_price => price.to_s || @product.price.to_s,
      :description => template("lib/ebay_templates/description.html.erb"),
      :condition_description => template("lib/ebay_templates/condition_description"),
      :listing_duration => "Days_#{duration}",
      :item_specifics => item_specifics,
      :listing_type => "FixedPriceItem",
      :title => title || @product.ebay_title,
      :currency => "USD",
      :country => "US",
      :postal_code => "21030",
      :global_shipping => "true",
      :payment_methods => ["PayPal", "VisaMC", "Discover"],
      :auto_pay => "true",
      :condition_id => @product.condition.to_s,
      :paypal_email_address => "info@EMAIL.com",
      :dispatch_time_max => 1,
      :picture_details => remote_images 
    })
  end

  def set_product
    @product = Product.find(@product_id)
  end

  def api
    @api ||= Ebay::Api.new
  end
end
\$\endgroup\$
  • \$\begingroup\$ I recommend using map and select to replace some of your each calls. \$\endgroup\$ – Nick McCurdy Aug 4 '14 at 6:11
  • \$\begingroup\$ @NicolasMcCurdy This looks like the start of an answer, you could maybe add a bit of info on why and how and you would have a good answer! \$\endgroup\$ – Marc-Andre Sep 4 '14 at 14:08
2
\$\begingroup\$

Since you are sub-classing ActiveModel::Model you can use you can use its ability to "magically" map an input hash to attributes:

def initialize(form={})
    super # magic!
    @product = set_product if @product_id
end

This works since you have created accessors for the properties :title, :price, :subtitle, :product_id, :duration.

Also as @nicolas-mccurdy mentioned you can replace each the enumerable methods map, select and reject. The rule of thumb being that you should only use each when you are solely concerned with the side-effects of a loop.

def item_specifics
    @product.properties.reject { |k,v| v == "0" }.map do |k,v|
        value = "#{v}#{PROPERTY_FORMATTER[k.to_sym]}"
        value = "included" if value == "1"
        Ebay::Types::NameValueList.new(name: k.to_s.for_humans, value: value)
    end
end

def remote_images
    # Side note - where does the variable `url` come from? Document it!
    @product.images.map do |i|
        url = "http://www.howmuchcomputer.com" + i
        remote_url = @api.upload_site_hosted_pictures({:external_picture_url => url})
        remote_images << remote_url.extract(ignore_errors: true)
    end.reverse
end

Also note the use of chaining - extremely common in Ruby - instead of creating intermediate variables or mutating.

The template method looks like it may be violation of MVC - models should not know about template or any kind of visual representation. Presenting data to users and dealing with user input is the concern of your controllers.

# @todo refactor into controller! Violation of concerns!
def template(path)
    ERB.new(File.open(path, "r").read).result(binding).to_s
end

Also you should consider the visibility of your methods, all methods which you do not consider part of the "public API" of a class should be private. These look like prime canidates:

def set_product
    @product = Product.find(@product_id)
end

def api
    @api ||= Ebay::Api.new
end

Addition

In general I would question the whole purpose of the class - "form classes" or form builders are pretty common in other frameworks such as Symfony2. IMHO they are dinosaur concepts from the days of ASP.net, that get filled with gluecode and cruft.

Just stick with good old MVC:

  • Views create forms and can be DRY-ed with partials and helpers. (or the awesome gem SimpleForm
  • Controllers deal with the user input and pass DATA to model.
  • Models deal with getting/persisting data from databases or external sources. They also handle validation. DRY them out with concerns.

If you want to refactor I would consider moving the relevant functionality into Listing - for example the post method.

item_specifics and remote_images is also an example of something that should probably belong to Product.

\$\endgroup\$
  • \$\begingroup\$ Add refactoring recommendation. \$\endgroup\$ – papirtiger May 8 '15 at 11:53

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.