Вопрос по refactoring, ruby, ruby-on-rails – Помогите в рефакторинге этого противного выражения Ruby if / else

10

Так что у меня есть это большое, волосатое утверждение if / else. Я передаю ему номер для отслеживания, а затем он определяет, какой это номер для отслеживания.

Как я могу упростить эту вещь? Конкретно желая уменьшить количество строк кодов.

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

Да, я знаю. Это противно.

Что значитcheck_response(number) делать? Кроме того, остальные могут проживать вget_tracking_service метод. Ewan Todd

Ваш Ответ

5   ответов
37

Попробуй это. Я переписал это с помощьюcase и регулярные выражения. Я также использовал:symbols вместо"strings" для возвращаемых значений, но вы можете изменить это обратно.

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
@radiospiel Я не думал об этом куске кода уже 3 года ... но, похоже, ты прав. Несмотря на то, что код ОП имеет длину == 20, он указан какuspsдаже если бы он уже был обработанcheck_response... Во всяком случае, я надеюсь, что ОП получил то, что им нужно :) jtb,andes
Ха! Эти вещи вневременны, как чтение шахматной головоломки в газете. David Foster
Я бы заменил исправление лямбды и обезьяны на: /^96(.ndom20 coming)$/ и /^[HK](.ndom10 coming)$/, предполагая,num_length длинаnumber John Douthat
@ ДэвидФостер без обид. Можете ли вы поверить, что этот ответ был написан 9 лет назад? jtbandes
Но это выглядит неправильно, в num_length == 18 || num_length == 20 случаев он ведет себя иначе, чем код OP. Или нет? radiospiel
6

является ли код отслеживания объектом ruby, вы можете также добавить вспомогательный код в определение этого класса:

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

Тогда ваше условное становится:

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

Одно упрощенное условие, когда вы работаете с подразумеваемой функцией кода отслеживания. Аналогично, если бы вы использовали циклический подход, ваш цикл также был бы чище:

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

или даже:

%w(ups? dhl? fedex?).each do |is_code|
  yield if number.send(is_code)
end
Я согласен с таким подходом, заключая в себе трудные для понимания условия для определения отправителя, что позволяет получить гораздо более читаемый код. Это сделает тесты намного более разумными и более легкими для понимания (например, для насмешек). Наконец, если один из отправителей изменяет логику для генерации номеров отслеживания, изменения в вашем коде минимальны. jkupferman
1

чем решение jtbandes, вам может понравиться это, поскольку оно немного более декларативно:

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

Я не имел дело с вызовом метода check_response здесь, так как я чувствую, что вы, вероятно, должны обработать это в другом месте (предполагая, что он делает что-то иное, чем возвращает имя службы отслеживания).

2

как будто он написан для скорости. Вы можете использовать minhash в качестве замены, но я думаю, что код довольно чистый и не требует рефакторинга. Rubyists, как правило, испытывают отвращение к ненужной структуре, но часто это необходимо для моделирования реальных ситуаций и / или повышения производительности. Ключевое слово должно быть ненужным.

Хотя вы можете быть правы, что иногда необходима дополнительная структура, этот код может быть написан более чисто. Смотри мой ответ. jtbandes
Это не похоже, что это было написано для скорости для меня. Я не вижу четких оптимизаций или чего-то еще. Мне кажется, это самый очевидный способ проверить кучу условий. Chuck
2

что это достаточно сложно, чтобы заслужить свой собственный метод.

Кстати, если длина равна 20, то оригинальная функция возвращаетcheck_response(n) возвращается, но затем пытается (и всегда потерпит неудачу) вернуться'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
+1 за включение тестов Sqeaky

Похожие вопросы