views:

486

answers:

5

So I have this big, hairy if/else statement. I pass a tracking number to it, and then it determines what type of tracking number it is.

How can I simplify this thing? Specifically wanting to reduce the number of lines of codes.

if num_length < 8
  tracking_service = false
else
  if number[1, 1] == 'Z'
    tracking_service = 'ups'
  elsif number[0, 1] == 'Q'
    tracking_service = 'dhl'
  elsif number[0, 2] == '96' && num_length == 22
    tracking_service = 'fedex'
  elsif number[0, 1] == 'H' && num_length == 11
    tracking_service = 'ups'
  elsif number[0, 1] == 'K' && num_length == 11
    tracking_service = 'ups'
  elsif num_length == 18 || num_length == 20
    check_response(number)
  else
    case num_length
    when 17
      tracking_service = 'dhlgm'
    when 13,20,22,30
      tracking_service = 'usps'
    when 12,15,19
      tracking_service = 'fedex'
    when 10,11
      tracking_service = 'dhl'
    else
      tracking_service = false  
    end  
  end
end

Yes, I know. It's nasty.

+2  A: 

This method looks like it was written for speed. You can use a minhash as a substitute, but I think the code is fairly clean and doesn't require a refactor. Rubyists tend to be disgusted by needless structure, but oftentimes it's needed to model real-world situations and/or provides a performance boost. The keyword should be needless.

Stefan Mai
While you may be correct that extra structure is necessary sometimes, this code could be written more cleanly. See my answer.
jtbandes
It doesn't look like it was written for speed to me. I don't see any clear optimizations or anything. It looks to me like the most obvious way to check a bunch of conditions.
Chuck
+11  A: 

Try this. I rewrote it using case and regular expressions. I also used :symbols instead of "strings" for the return values, but you can change that back.

tracking_service = case number
  when /^.Z/ then :ups
  when /^Q/ then :dhl
  when /^96.{20}$/ then :fedex
  when /^[HK].{10}$/ then :ups
else
  check_response(number) if num_length == 18 || num_length == 20
  case num_length
    when 17 then :dhlgm
    when 13, 20, 22, 30 then :usps
    when 12, 15, 19 then :fedex
    when 10, 11 then :dhl
    else false
  end
end
jtbandes
I'd replace the lambdas and monkey patching with:/^96(.{20})$/ and /^[HK](.{10})$/, assuming `num_length` is the length of `number`
John Douthat
Excellent point, John, thanks.
jtbandes
Seriously love this. Spot on for what I was looking for. :)
Shpigford
+1  A: 

Whilst longer than jtbandes solution, you might like this as it's a bit more declarative:

class Condition
  attr_reader :service_name, :predicate

  def initialize(service_name, &block)
    @service_name = service_name
    @predicate = block
  end
end

CONDITIONS = [
  Condition.new('ups')   { |n| n[1] == 'Z' },
  Condition.new('dhl')   { |n| n[0] == 'Q' },
  Condition.new('fedex') { |n| n[0..1] == '96' && n.size == 22 },
  Condition.new('ups')   { |n| n[0] == 'H' && n.size == 11 },
  Condition.new('ups')   { |n| n[0] == 'K' && n.size == 11 },
  Condition.new('dhlgm') { |n| n.size == 17 },
  Condition.new('usps')  { |n| [13, 20, 22, 30].include?(n.size) },
  Condition.new('fedex') { |n| [12, 15, 19].include?(n.size) },
  Condition.new('dhl')   { |n| [10, 11].include?(n.size) },
]

def tracking_service(tracking_number)
  result = CONDITIONS.find do |condition|
    condition.predicate.call(tracking_number)
  end

  result.service_name if result
end

I haven't dealt with the check_response method call here as I feel you should probably handle that elsewhere (assuming it does something other than return a tracking service name).

npad
+2  A: 

Depending on whether or not the tracking code is a ruby object, you could also put helper's in it's class definition:

class TrackingCode < String 
  # not sure if this makes sense for your use case
  def ups?
    self[1,1] == 'Z'
  end
  def dhl?
    self[0,1] == 'Q'
  end
  def fedex?
    self.length == 22 && self[0, 2] == '96'
  end
  # etc...
end

Then your conditional becomes:

if number.ups?
  # ...
elsif number.dhl?
  # ...
elseif number.fedex?
end

One simplified conditional where you are operating on the implied feature of the tracking code. Likewise, if you were to take a looping approach, your loop would also be cleaner:

%w(ups? dhl? fedex?).each do |is_code|
  return if number.send(is_code)
end

or even:

%w(ups? dhl? fedex?).each do |is_code|
  yield if number.send(is_code)
end
frogstarr78
I agree with this approach, encapsulating the difficult to follow conditionals for determining sender allows for much more readable code. This will make tests much more reasonable and easier to understand (e.g. for mocking). Finally if one of the senders changes the logic for generating tracking numbers the changes to your code are minimal.
jkupferman
+1  A: 

I believe this is sufficiently complex to deserve its own method.

BTW, if the length is 20 then the original function returns whatever check_response(n) returns, yet then attempts (and will always fail) to return 'usps'.

@lenMap = Hash.new false
@lenMap[17] = 'dhlgm'
@lenMap[13] = @lenMap[20] = @lenMap[22] = @lenMap[30] = 'usps'
@lenMap[12] = @lenMap[15] = @lenMap[19] = 'fedex'
@lenMap[10] = @lenMap[11] = 'dhl'       

def ts n
  len = n.length
  return false if len < 8
  case n
    when /^.Z/
      return 'ups'
    when /^Q/
      return 'dhl'
    when /^96....................$/
      return 'fedex'
    when /^[HK]..........$/
      return 'ups'
  end
  return check_response n if len == 18 or len == 20
  return @lenMap[len]
end

# test code...
def check_response n
  return 'check 18/20 '
end
%w{ 1Zwhatever Qetcetcetc 9634567890123456789012 H2345678901
    K2345678901 hownowhownowhownow hownowhownowhownow90
    12345678901234567
    1234567890123
    12345678901234567890
    1234567890123456789012
    123456789012345678901234567890
    123456789012
    123456789012345
    1234567890123456789
    1234567890
    12345678901 }.each do |s|
      puts "%32s  %s" % [s, (ts s).to_s]
    end
DigitalRoss