5
\$\begingroup\$

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
\$\endgroup\$
3
\$\begingroup\$

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
\$\endgroup\$
3
  • \$\begingroup\$ Thank you! I'll study what you did here and fill in some holes in my Ruby knowledge. \$\endgroup\$
    – neontapir
    Jan 24 '12 at 19:22
  • \$\begingroup\$ Rather than thanking me, would you consider voting up or accepting my answer? :) \$\endgroup\$ Feb 1 '12 at 6:27
  • \$\begingroup\$ I believe that => is known as a “hashrocket” because it’s used in hashes (associative arrays), not because it’s associated with the number sign. And there’s nothing wrong with using it in sample output IMHO. \$\endgroup\$ Feb 13 '19 at 15:29
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Yes, very helpful. RSpec is an excellent suggestion, as is map. I am blind to those features I don't know exist. :) \$\endgroup\$
    – neontapir
    Nov 1 '11 at 16:08
  • \$\begingroup\$ You are also correct that there is a bug in my algorithm. \$\endgroup\$
    – neontapir
    Nov 1 '11 at 16:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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