current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I'm new to Ruby. I normally sling code in C#. What could I do to make this simple Soundex class more Rubyesque?

class Surname
  attr_accessor :value

  def initialize(input)
    @value = input
  end

  def soundex
    result = ''
    @value.chars.drop(1).each do |s|
      number = soundex_value(s).to_s
      result << number unless result[-1,1] == number
    end
    @value.chars.first << result.ljust(3,'0')
  end

  def soundex_value(s)
    case s
    when /[bfpv]/
      1
    when /[cgjkqsxz]/ 
      2
    when /[dt]/ 
      3
    when /l/ 
      4
    when /[mn]/ 
      5
    when /r/ 
      6
    else ''
    end
  end
end

def print_name(input)
  surname = Surname.new(input)
  puts(surname.value + ' => ' + surname.soundex)
end

['Smith', 'Johnson', 'Williams', 'Jones', 'Brown'].each do |s|
  print_name s
end

The output is: 

Smith => S530
Johnson => J525
Williams => W452
Jones => J520
Brown => B650
share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

Per your request, your code converted to the Ruby way perhaps would be as follows.

A small point: in our output, in the Ruby world, we would avoid the => fat comma which Rubyists (at least in the English-speaking world) call a 'hash-rocket' because:

  • It reads like Ruby's (older) hash accessing syntax.
  • In Ruby language documentation (e.g., see Array), the similar #=>, comprising a hash to introduce a comment, followed by a hash-rocket (presumably, this gave us the name), shows us the result (or value) of an individual line of code—the two meanings conflict.

BTW, we also would avoid ->, dash-greater-than, which in Ruby (syntax) generates a lambda. 

class Soundex < String
  IGNORED_BEGINNING_LENGTH = 1
  MINIMUM_LENGTH = 3

  CASES = [ # Keep order.
    /[bfpv]/,
    /[cgjkqsxz]/,
    /[dt]/,
    /l/,
    /[mn]/,
    /r/,
  ]
  CASES_LENGTH = CASES.length

  def initialize(surname)
    a = surname.split ''
    kept    = a.take(IGNORED_BEGINNING_LENGTH).join ''
    indices = a.drop(IGNORED_BEGINNING_LENGTH).map do |e|
      (0...CASES_LENGTH).detect{|i| e =~ (CASES.at i)}
    end.compact
# Adjust to one-based notation; collapse repetition; right-pad with zeros.
    digits = indices.map(&:succ).join('').squeeze.ljust MINIMUM_LENGTH, '0'
    super kept + digits
  end

  def self.show(s) "#{s}: #{new s}" end
end

names = %w[Smith Johnson Williams Jones Brown Atchison]
names.each{|s| puts Soundex.show s}

gives the results

Smith: S530
Johnson: J525
Williams: W452
Jones: J520
Brown: B650
Atchison: A325
share|improve this answer
    
Thank you! I'll study what you did here and fill in some holes in my Ruby knowledge. –  neontapir Jan 24 '12 at 19:22
    
Rather than thanking me, would you consider voting up or accepting my answer? :) –  MarkDBlackwell Feb 1 '12 at 6:27
    
Thanks for accepting my answer. :) –  MarkDBlackwell Jun 8 '13 at 11:39
add comment
up vote 1 down vote

First of all: install RSpec and write some tests if you haven't already. That will make sure you don't break anything while refactoring. (And then do all development test-first if you don't already.) Also, there are existing Ruby Soundex implementations. You might want to use one instead of writing your own.

For the rest of it:

  • I think your implementation is incorrect! Try this: does "Atchison" yield "A322"? It should.

  • Why do you need a separate class for Surname? It may make sense to have a module with the Soundex algorithm that either gets mixed into Strings as necessary or calls the method with function syntax (e.g. Soundex::encode last_name). I'm not sure.

  • If you do need a separate class for Surname, perhaps it should extend String, or delegate to its String member, so that you can easily call String methods on it. But I think a plain old String with a module included would work better.

  • @value seems like a nondescriptive name. Perhaps @name or something?

  • You may not need the attr_accessor, since @surname.to_s would be more idiomatic than @surname.value.

  • Instead of that @value.chars.drop(1).each block, you may want to use map.

  • Regular expressions may not be the right thing for the case expression; you may want when 'b', 'p' instead. Or you may want to drop the case entirely and have a hash: {'b' => 1, 'c' => 2, 'd' => 3, 'f' => 1, ... }. Or I seem to recall that Array#assoc might be useful here...

  • Since you're calling to_s on soundex_value the only time you use it, maybe it should just return a string in the first place.

Good luck! I hope these suggestions are helpful.

share|improve this answer
    
Yes, very helpful. RSpec is an excellent suggestion, as is map. I am blind to those features I don't know exist. :) –  neontapir Nov 1 '11 at 16:08
    
You are also correct that there is a bug in my algorithm. –  neontapir Nov 1 '11 at 16:19
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.