How can I DRY this code and clean up my model?

I have the following two methods for a Number model.

def track
  number = sanitize(tracking)

  case determine_type(number)
  when 'UPS'
    tracker = ups.track(:tracking_number => number)
    self.carrier              = Carrier.where(:name => 'UPS').first
    self.service              = tracker.service_type
    self.destination_country  = tracker.destination_country
    self.destination_state    = tracker.destination_state
    self.destination_city     = tracker.destination_city
    self.origin_country       = tracker.origin_country
    self.origin_state         = tracker.origin_state
    self.origin_city          = tracker.origin_city
    self.signature            = tracker.signature_name
    self.scheduled_delivery   = tracker.scheduled_delivery_date
    self.weight               = tracker.weight

    tracker.events.each do |event|
      new_event             = Event.new
      new_event.number      = self
      new_event.city        = event.city
      new_event.state       = event.state
      new_event.postalcode  = event.postal_code if event.postal_code
      new_event.country     = event.country
      new_event.status      = event.name
      new_event.status_code = event.type
      new_event.occured_at  = event.occurred_at

      new_event.save
    end
  end

  save
end

def update_number

  case determine_type(number)
  when 'UPS'
    tracker = ups.track(:tracking_number => tracking)
    self.carrier              = Carrier.where(:name => 'UPS').first
    self.service              = tracker.service_type
    self.destination_country  = tracker.destination_country
    self.destination_state    = tracker.destination_state
    self.destination_city     = tracker.destination_city
    self.origin_country       = tracker.origin_country
    self.origin_state         = tracker.origin_state
    self.origin_city          = tracker.origin_city
    self.signature            = tracker.signature_name
    self.scheduled_delivery   = tracker.scheduled_delivery_date
开发者_JAVA百科    self.weight               = tracker.weight

    last_event = self.events.ordered.first.occured_at

    tracker.events.each do |event|
      if last_event and (event.occurred_at > last_event)
        new_event             = Event.new
        new_event.number      = self
        new_event.city        = event.city
        new_event.state       = event.state
        new_event.postalcode  = event.postal_code if event.postal_code
        new_event.country     = event.country
        new_event.status      = event.name
        new_event.status_code = event.type
        new_event.occured_at  = event.occurred_at

        new_event.save
      end
    end
  end
  save
end

As you can see, a lot of code is duplicated. And the problem becomes when I start adding the dozen or so other carriers (FedEx, USPS, DHL, etc)...my Number model gets big and hairy fast.

The only real difference between track and update_number is that update_number as an if comparison around the events to check if the events from the carrier are more recent than the latest events I have stored in the database, using that if last_event and (event.occurred_at > last_event) line.

So how can I clean up this code so my model doesn't get so fat?

A couple of things I would suggest:

1. Look at the Strategy Pattern, even though Ruby doesn't have interfaces, but it'll give you some ideas on how to better design this functionality (google for Strategy Pattern for Ruby to find alternatives). Basically, you'd want to have separate classes to handle your switch case statements and call the appropriate one at runtime.
2. Try to separate the tracker handling code from the events handling code. E.g., I would consider moving the event creation/saving logic for each of the tracker events to a separate class (or even to the Tracker entity, if you have one).

Hope this helps.

This should be solved using the Strategy pattern. For each possible tracker (DHL, UPS, ...) that will handle the creating and updating appropriately.

So that would become something like:

class Number

  def track
    tracker = get_tracker(tracking)
    tracker.create_tracking(self)
    save
  end

  def update_number
    tracker = get_tracker(tracking)
    tracker.update_tracking(self)
    save
  end

  def get_tracker(tracking)
    tracking_number = sanitize(tracking)
    case determine_type(tracking_number)
    when 'UPS'
      UPSTracker.new(tracking_number)
    when 'DHL'
      DHLTracker.new(tracking_number)
    end
  end      
end

class UPSTracker

  def initialize(tracking_number)
    @tracking_number = tracking_number
  end

  def create_tracking(number)
    tracker = ups.track(:tracking_number => number)
    update_number_properties(number, tracking)

    # store events
    tracker.events.each do |event|
      create_new_event(event)
    end
  end

  def update_tracking(number)
    tracker = ups.track(:tracking_number => number)
    update_number_properties(number, tracking)

    last_event = self.events.ordered.first.occured_at

    tracker.events.each do |event|
      if last_event and (event.occurred_at > last_event)
        create_new_event(event)
      end
    end
  end

  protected

  def update_number_properties
    number.carrier = Carrier.where(:name => 'UPS').first
    number.service              = tracker.service_type
    number.destination_country  = tracker.destination_country
    number.destination_state    = tracker.destination_state
    number.destination_city     = tracker.destination_city
    number.origin_country       = tracker.origin_country
    number.origin_state         = tracker.origin_state
    number.origin_city          = tracker.origin_city
    number.signature            = tracker.signature_name
    number.scheduled_delivery   = tracker.scheduled_delivery_date
    number.weight               = tracker.weight
  end

  def create_new_event
    new_event             = Event.new
    new_event.number      = self
    new_event.city        = event.city
    new_event.state       = event.state
    new_event.postalcode  = event.postal_code if event.postal_code
    new_event.country     = event.country
    new_event.status      = event.name
    new_event.status_code = event.type
    new_event.occured_at  = event.occurred_at
    new_event.save
  end
end

This code could be further improved, I imagine the creating of events and trackings will be shared over the different carriers. So a shared base-class maybe. Secondly, in two methods inside Number we call the get_tracker: probably this tracker can be decided upon creation time (of the Number-instance) and should be fetched once and stored inside an instance variable.

Furthermore I would like to add that names should be meaningful, so a class Number does not sound meaningful enough to me. A name should express intent, and preferably match the names and concepts from your problem domain.

Something about your code doesn't make sense to me, the organization is a little strange and I don't have enough context on what your app looks like. The OOP-ish way to accomplish something like this in rails is pretty easy; you could also roll your own Strategy, Adapter, or even a Builder pattern to do this.

But, there is some low hanging fruit, you can refactor your code so the common parts are less obtrusive -- this is a little better but, create_events is still very case'y:

def track
  create_events
end

def update_number
  create_events {|e| last_event and (event.occurred_at > last_event) }
end

def create_events(&block)
  case determine_type(number)
  when 'UPS'
    tracker = ups.track(:tracking_number => tracking)
    self.carrier = Carrier.where(:name => 'UPS').first
    self.assign_tracker(tracker)
  end
  tracker.events.each do |e|
    self.create_event(e) unless (block_given? && !block.call(e))
  end
  save
end

def assign_tracker(tracker)
  self.service              = tracker.service_type
  self.destination_country  = tracker.destination_country
  self.destination_state    = tracker.destination_state
  self.destination_city     = tracker.destination_city
  self.origin_country       = tracker.origin_country
  self.origin_state         = tracker.origin_state
  self.origin_city          = tracker.origin_city
  self.signature            = tracker.signature_name
  self.scheduled_delivery   = tracker.scheduled_delivery_date
  self.weight               = tracker.weight
end

def create_event(event)
  new_event             = Event.new
  new_event.number      = self
  new_event.city        = event.city
  new_event.state       = event.state
  new_event.postalcode  = event.postal_code if event.postal_code
  new_event.country     = event.country
  new_event.status      = event.name
  new_event.status_code = event.type
  new_event.occured_at  = event.occurred_at
  new_event.save
end