55502f40dc8b7c769880b10874abc9d0

any ideas on making this more concise?

stuff.each do |name|
  name = name.downcase
  #size
  if name =~ /(double|twin)/
    size = 2
  elsif name =~ /(single)/
    size = 1
  else name =~ /[0-9]/
    size = name.match(/[0-9]{1,2}/)[0]
  end
  
  #type
  if name =~ /(male)/
    type = "Male Dorm"
  elsif name =~ /(female)/
    type = "Female Dorm"
  elsif name =~ /(double)/
    type = "Double Bed"
  elsif name =~ /(twin)/
    type = "Twin Bed"
  elsif name =~ /(single)/
    type = "Single Bed"
  else
    type = "Mixed Dorm"
  end
  
  if name =~ /(private)/
    priv = '1'
  else
    priv = '0'
  end

  if name =~ /(ensuite)/
    ensuite = '1'
  else
    ensuite = '0'
  end
  
  name = name.gsub(/(dorm|male|female|double|single|beds|bed|private|ensuite|twin|[\d\(\)*])/,'').squeeze(' ')
  
  puts "Name: #{name}, Size: #{size}, #{type}, #{priv}, #{ensuite}"
end

Refactorings

No refactoring yet !

E8f0d6e9bc8bca695b3c5bdf75cdcc03

Martin Plöger

September 10, 2009, September 10, 2009 14:05, permalink

1 rating. Login to rate!

What do you exactly want with that: [\d\(\)*???
name = name.gsub(/(dorm|male|female|double|single|beds|bed|private|ensuite|twin|[\d\(\)*])/,'').squeeze(' ')
Inside the brackets the letters have other meanings... Could you give an example so that this line could be refactored?

stuff.each do |name|
  name = name.downcase
  
  #size
  size = case name
           when /double|twin/ then 2
           when /single/      then 1
           else name[/[0-9]+/] || 1 # Set size as a last resort to 1 instead of nil
         end

  #type
  type = case name
           when /female/ then 'Female Dorm'
           when /male/   then 'Male Dorm'
           when /double/ then 'Double Bed'
           when /twin/   then 'Twin Bed'
           when /single/ then 'Single Bed'
           else               'Mixed Dorm'
         end

  #You don't need to return Strings here like you did: '1' => #{x} calls x.to_s on it  
  priv, ensuite = name =~ /private/ ? 1 : 0, name =~ /ensuite/ ? 1 : 0
  #If boolean results (true/false) are okay: priv, ensuite = %w(private ensuite).map { |x| name.include? x }
  
  #What do you exactly want with that: [\d\(\)*???
  name = name.gsub(/(dorm|male|female|double|single|beds|bed|private|ensuite|twin|[\d\(\)*])/,'').squeeze(' ')
  
  puts "Name: #{name}, Size: #{size}, #{type}, #{priv}, #{ensuite}"
end
E8f0d6e9bc8bca695b3c5bdf75cdcc03

Martin Plöger

September 10, 2009, September 10, 2009 14:31, permalink

1 rating. Login to rate!

Another way might be a mapping-table or a hash which maps the name-parts to the matching results (size, type) and you iterate over the table/hash-keys and check if they match and then take the results.
But sometimes this might be to cryptic for so few cases:

mapping = {'double' => ['Double Bed', 2], 'twin' => ['Twin Bed', 2], 'single' => ['Single Bed', 1], 'female' => 'Female Dorm', ... }

type, size = mapping.find { |k, v| name.include? k }
size ||= name[/[0-9]+/] || 1
priv, ensuite = name =~ /private/ ? 1 : 0, name =~ /ensuite/ ? 1 : 0

name = name.gsub(/(dorm|male|female|double|single|beds|bed|private|ensuite|twin|[\d\(\)*])/,'').squeeze(' ')

puts "Name: #{name}, Size: #{size}, #{type}, #{priv}, #{ensuite}"
Cbac6b08f6f6aebe2dee197dc34e0d74

Josiah

September 10, 2009, September 10, 2009 14:41, permalink

No rating. Login to rate! 
sizes = {
	:double => 2,
	:twin => 2,
	:single => 1
}

types = {
	:male => "Male Dorm",
	:female => "Female Dorm",
	:double => "Double Bed",
	:twin => "Twin Bed",
	:single => "Single Bed",
}
types.default = "Mixed Dorm"

stuff.each do |name|
	name.downcase!
	size = sizes[name.to_sym] || name[/[0-9]+/] || 1
	type = types[name.to_sym]
	
	priv, ensuite = name =~ /private/ ? 1 : 0, name =~ /ensuite/ ? 1 : 0
	name = name.gsub(/(dorm|male|female|double|single|beds|bed|private|ensuite|twin|[\d\(\)*])/,'').squeeze(' ')
	puts "Name: #{name}, Size: #{size}, #{type}, #{priv}, #{ensuite}"
end
A8d3f35baafdaea851914b17dae9e1fc

Adam

September 10, 2009, September 10, 2009 16:03, permalink

1 rating. Login to rate! 
class Room
  SIZES = { :double => 2, :twin => 2, :single => 1 }

  attr_reader :name, :size
  
  def initialize(name)
    @name = name.downcase
    @size = SIZES[self.name.to_sym] || name[/\d+/] || 1
  end
  
  def type
    prefix = name[/male|female|double|twin|single/] || 'mixed'
    type   = prefix.captialize
    type  << SIZES.has_key?(prefix.to_sym) ? 'Bed' : 'Dorm'
  end
  
  def priv
    name =~ /private/ ? '1' : '0'
  end
  
  def ensuite
    name =~ /ensuite/ ? '1' : '0'
  end
  
  def to_s
    name = self.name.gsub(/(dorm|male|female|double|single|beds|bed|private|ensuite|twin|[\d\(\)*])/,'').squeeze(' ')
    "Name: #{name}, Size: #{size}, #{type}, #{priv}, #{ensuite}"
  end
end


stuff.each { |name| puts Room.new(name) }
F4192eeb4b26e96deab8b5c68926105d

Muke Tever

September 11, 2009, September 11, 2009 06:00, permalink

2 ratings. Login to rate!

Not a refactoring (the above seem to cover it), but sort of a bug warning:

name = "female"

if name =~ /male/ 
  # (Doing stuff for the male case...)
  # /male/ matches "female" so this will get called for females too
  # You'll be ending up with a lot of girls in the guys' dorm.
elsif name =~ /female/
  # (Theoretically doing other stuff for the female case...)
  # ...but this won't get called because we already matched /male/
  # Best to switch the order of these conditions if you do regexp matches
end
E8f0d6e9bc8bca695b3c5bdf75cdcc03

Martin Plöger

September 11, 2009, September 11, 2009 09:16, permalink

No rating. Login to rate!

I just updated my solution and changed the order of the Regexes so that it first checks for /female/ and if it doesn't match checks for /male/. @Muke: Thanks for the hint!

#...
case name
           when /female/ then 'Female Dorm'
           when /male/   then 'Male Dorm'
#...

Your refactoring





Format Copy from initial code

or Cancel