Requesting product data from an eBay webform

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 url})
remote_images 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",
:

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.

StackExchange Code Review Q#53919, answer score: 2