Как я могу очистить этот код Ruby?

ProgramBox

Как я могу очистить этот код Ruby?

Post author:admin
Запись опубликована:24 марта, 2023
Post category:Вопросы по программированию

#ruby-on-rails #ruby #refactoring

#ruby-on-rails #ruby #рефакторинг

Вопрос:

Проблема

Я хочу проанализировать строки текстового поля, чтобы сгенерировать из него элементы.

У меня это работает, но выглядит ужасно, как грех.

Как я могу очистить этот беспорядок «if / else»?

Код

 class LineParser
  def self.parse(line)
    line_matches = line.chomp.match(/(?<name>[[:print:]] ) $(?<price>d .*d*)(  ?(?<description_text>[^ #]([[:print:]][^ #])*))?(  ?( (?<quantity>d )))?(  ?#(?<cat1>[[:print:]][^#]*))?$/)
    return matches_to_hash(line_matches)
  end
  def self.matches_to_hash(matches)
    hash = {}
    keys = [:name, :price, :description_text, :quantity, :cat1]
    keys.each do |key|
      if key == :price
        hash[key] = matches[key].to_f
      elsif key == :quantity
        if matches[key].nil?
          hash[key] = 1
        else
          hash[key] = matches[key].to_i
        end
      else
        hash[key] = matches[key]
      end
    end
    hash
  end
end

Ответ №1:

Это

 if matches[key].nil?
  hash[key] = 1
else
  hash[key] = matches[key].to_i
end

эквивалентно

 hash[key] = (matches[key] || 1).to_i

И для цепочек if / else с более чем парой ветвей, возможно, более подходящим является оператор case.

Ответ №2:

Удален явный возврат из синтаксического анализа.

 class LineParser
  def self.parse(line)
    line_matches = line.chomp.match(/(?<name>[[:print:]] ) $(?<price>d .*d*)(  ?(?<description_text>[^ #]([[:print:]][^ #])*))?(  ?( (?<quantity>d )))?(  ?#(?<cat1>[[:print:]][^#]*))?$/)
    matches_to_hash(line_matches)
  end
  def self.matches_to_hash(matches)
    {
      :price            => matches[:price].to_f,
      :quantity         => (matches[:quantity] || 1).to_i,
      :name             => matches[:name],
      :description_text => matches[:description_text],
      :cat1             => matches[:cat1]
    }
  end
end

1. Черт возьми, это прекрасно. Я ненавидел делать всю эту штуку с хэшем возврата hash = {}, и я знал, что, должно быть, был лучший способ. Это оно! Спасибо.

2. ммм ruby 🙂 Если вы находитесь в ситуации, когда литералы не имеют смысла, вы также можете использовать Object#tap .

3. количество полей (5) находится на границе. Например, если бы у меня было 10 полей, я бы определенно использовал цикл для полей, но это нормально для 5.

4. @tokland согласен и, вероятно, начнет извлекать константы и другой рефакторинг

5. При создании хэша {:thing1 => 1, :thing2 => 2} необходимо ставить запятую

Ответ №3:

Чтобы привести в порядок matches_to_hash, я бы сделал:

 def self.matches_to_hash(matches)
  hash = {}
  hash[:name] = matches[:name]
  hash[:price] = matches[:price].to_f
  hash[:description_text] = matches[:description_text]
  hash[:quantity] = matches[:quantity].nil? ? 1 : matches[:quantity].to_i
  hash[:cat1] = matches[:cat1]
  hash
end

Но, я думаю, я бы просто изменил parse на:

 def self.parse(line)
  line_matches = line.chomp.match(/([[:print:]] ) $(d .*d*)(  ?([^ #]([[:print:]][^ #])*))?(  ?( (d )))?(  ?#([[:print:]][^#]*))?$/)
  hash = {}
  hash[:name] = $1
  hash[:price] = $2.to_f
  hash[:description_text] = $3
  hash[:quantity] = $4.nil? ? 1 : $4.to_i
  hash[:cat1] = $5
  hash
end

1. зачем создавать хэш в стиле PHP?

2. Отличный момент — я изначально поместил совпадения как названные, потому что это дало мне объект, подобный хэшу, но если мне нужно манипулировать им, прежде чем я помещу его в хэш, тогда в этом нет особого смысла.

3. обычный php-стиль: hash = {}; hash[:a] = 1. Другие языки: hash = {:a => 1}.

4. А, ладно. Ruby поддерживает оба способа, поэтому я не настолько придирчив. Работает все, что выглядит читаемым.

Ответ №4:

 class LineParser
  def self.parse(line)
    line_matches = line.chomp.match(/(?<name>[[:print:]] ) $(?<price>d .*d*)(  ?(?<description_text>[^ #]([[:print:]][^ #])*))?(  ?( (?<quantity>d )))?(  ?#(?<cat1>[[:print:]][^#]*))?$/)
    return matches_to_hash(line_matches)
  end
  def self.matches_to_hash(matches)
    hash = {}
    [:name, :price, :description_text, :quantity, :cat1].each do |key|
      case key
       when :price
         hash[key] = matches[key].to_f
       when :quantity
          hash[key] = 1 if matches[key].nil?
          hash[key] = matches[key].to_i unless matches[key].nil?
       else
         hash[key] = matches[key]
     end
     hash
  end
end

Ответ №5:

Я не совсем уверен, почему вы перебираете ключи, когда каждый ключ делает что-то другое. Почему бы просто не обработать их один за другим?

 class LineParser
  def self.parse(line)
    line_matches = line.chomp.match(/(?<name>[[:print:]] ) $(?<price>d .*d*)(  ?(?<description_text>[^ #]([[:print:]][^ #])*))?(  ?( (?<quantity>d )))?(  ?#(?<cat1>[[:print:]][^#]*))?$/)
    return matches_to_hash(line_matches)
  end
  def self.matches_to_hash(matches)
    hash = {}
    hash[:price] = matches[:price].to_f
    hash[:quantity] = (matches[:quantity] || 1).to_i
    hash[:name] = matches[:name]
    hash[:description_text] = matches[:description_text]
    hash[:cat1] = matches[:cat1]
    hash
  end
end

Примечание: я также украл умную часть, которую Тило опубликовал о количестве.

1. это выглядит хорошо, но нет необходимости создавать хэш, обновляя его. Просто создайте хэш за один шаг.

2. Да, оглядываясь назад, я тоже не уверен, почему. Наверное, я думал, что это уменьшит дублирование и упростит код. Чувак, я был неправ! Спасибо за ваше отличное предложение.