CODEWITHSUNDEEP
ruby-on-railsrubyrefactoring
John Gallagher
John Gallagher

Reputation: 6268

How can I clean up this Ruby code?

The Problem

I want to parse lines of a text box in order to generate items from it.

I've got it working but it looks ugly as sin.

How can I clean up that "if/else" mess?

Code

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

Upvotes: 1

Views: 448

Answers (5)

user1976
user1976

Reputation:

Removed explicit return from parse.

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

Upvotes: 3

ean5533
ean5533

Reputation: 8994

I'm not entirely certain why you're looping through the keys when each key does something different. Why not just handle them one by one?

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

Note: I also stole the clever bit that Thilo posted about quantity.

Upvotes: 1

drummondj
drummondj

Reputation: 1483

To tidy matches_to_hash I would do:

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

But, I think I would just change parse to:

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

Upvotes: 2

davidb
davidb

Reputation: 8954

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

Upvotes: 1

Thilo
Thilo

Reputation: 17735

This

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

is equivalent to

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

And for if/else chains with more than a couple of branches, maybe a case statement is more appropriate.

Upvotes: 3

Related Questions