5
\$\begingroup\$

I'm still learning Ruby and I made the following function to convert "N" or "SE" into "NORTH" or "SOUTH EAST" respectively. The function works and the tests all pass, but the code seems to be overly complicated. Any suggestions on how to clean it up and make it more Ruby-ish?

Code

module Example 
  class Utils
    CARDINAL_PATTERN = /^([NS]?)([EW]?)$/
    CARDINAL_DB = {'N'=>'NORTH', 'S'=>'SOUTH', 'E'=>'EAST', 'W'=>'WEST' }
    def correct_cardinal word
      clean_word = word.gsub(/\p{Punct}/, '').upcase
      match = CARDINAL_PATTERN.match(clean_word)
      if match
        if (not match[1].empty?) and (not match[2].empty?)
          CARDINAL_DB[match[1]] + " " + CARDINAL_DB[match[2]]
        elsif (not match[1].empty?)
          CARDINAL_DB[match[1]]
        elsif (not match[2].empty?)
          CARDINAL_DB[match[2]]
        else
          ""
        end
      else
        word
      end
    end
  end 
end

Tests

describe Example::Utils do
  let(:utils) { Example::Utils.new}

  describe '#correct_cardinal' do
    it 'should correct S' do
      expect(utils.correct_cardinal('S')).to eq "SOUTH"
    end 

    it 'should correct N' do
      expect(utils.correct_cardinal('N')).to eq "NORTH"
    end 

    it 'should correct E' do
      expect(utils.correct_cardinal('E')).to eq "EAST"
    end 

    it 'should correct W' do
      expect(utils.correct_cardinal('W')).to eq "WEST"
    end 

    it 'should strip single period' do
      expect(utils.correct_cardinal('W.')).to eq "WEST"
    end 

    it 'should correct SE' do
      expect(utils.correct_cardinal('SE')).to eq "SOUTH EAST"
    end 

    it 'should correct SW' do
      expect(utils.correct_cardinal('SW')).to eq "SOUTH WEST"
    end 

    it 'should correct NE' do
      expect(utils.correct_cardinal('NE')).to eq "NORTH EAST"
    end 

    it 'should correct NW' do
      expect(utils.correct_cardinal('NW')).to eq "NORTH WEST"
    end 

    it 'should strip multiple periods' do
      expect(utils.correct_cardinal('N.W.')).to eq "NORTH WEST"
    end 

    it 'should not correct 1' do
      expect(utils.correct_cardinal('1')).to eq "1"
    end 

    it 'should not ordinalize eggs' do
      expect(utils.correct_cardinal('EGGS')).to eq "EGGS"
    end 

  end

end
\$\endgroup\$
4
\$\begingroup\$

Some notes:

  • I'd leave some room between the constants and the method definitions (subjective).

  • if (not match[1].empty?) and (not match[2].empty?). Parenthenses not necessary. In any case, for boolean operations, use !, &&, ||.

  • When you have so many similar ifs, they can usually be refactored into something else (usually, with no conditionals at all, but array processing).

First, with your code as reference mind, I'd write:

module Example 
  class Utils
    CARDINAL_PATTERN = /^([NS]?)([EW]?)$/
    CARDINAL_DB = {'N' => 'NORTH', 'S' => 'SOUTH', 'E' => 'EAST', 'W' => 'WEST'}

    def correct_cardinal(word)
      clean_word = word.gsub(/\p{Punct}/, '').upcase
      match = CARDINAL_PATTERN.match(clean_word)
      if match
        cardinals = match.captures.reject(&:empty?)
        CARDINAL_DB.values_at(*cardinals).join(" ")
      else
        word
      end
    end
  end 
end

On a second refactor, as @janos pointed out, you can build the complete hash of {CARDINALS => NAME} (well, it's so short you could just write down all its values, but let's build it programatically to see a way to do it).

module Example 
  class Utils
    CARDINALS = {'N' => 'NORTH', 'S' => 'SOUTH', 'E' => 'EAST', 'W' => 'WEST'} 
    # DIRECTIONS: {"NE"=>"NORTH EAST", "NW"=>"NORTH WEST", "N"=>"NORTH", ...}
    DIRECTIONS = ["N", "S", ""].product(["E", "W", ""]).map do |cardinals|
      name = CARDINALS.values_at(*cardinals).compact.join(" ")
      [cardinals.join, name]
    end.to_h

    def correct_cardinal(word)
      clean_word = word.gsub(/\p{Punct}/, '').upcase
      DIRECTIONS.fetch(clean_word, word)
    end
  end
end
\$\endgroup\$
  • \$\begingroup\$ The first option is something closer to that I wanted to do, but wasn't familiar enough with the ruby APIs. \$\endgroup\$ – pgreen2 Jul 20 '14 at 10:46
3
\$\begingroup\$

All the round brackets are unnecessary here:

if (not match[1].empty?) and (not match[2].empty?)
  CARDINAL_DB[match[1]] + " " + CARDINAL_DB[match[2]]
elsif (not match[1].empty?)
  CARDINAL_DB[match[1]]
elsif (not match[2].empty?)
  CARDINAL_DB[match[2]]

Another solution can be pre-populating CARDINAL_DB with all the possible values:

CARDINAL_DB = {'N'=>'NORTH', 'S'=>'SOUTH', 'E'=>'EAST', 'W'=>'WEST' }
['N', 'S'].each { |ns|
    ['E', 'W'].each { |ew|
        CARDINAL_DB[ns + ew] = CARDINAL_DB[ns] + ' ' + CARDINAL_DB[ew]
    }
}

This will simplify the correct_cardinal method a bit:

def correct_cardinal word
  clean_word = word.gsub(/\p{Punct}/, '').upcase
  match = CARDINAL_PATTERN.match(clean_word)
  if match
      CARDINAL_DB[match[1] + match[2]]
  else
      word
  end
end
\$\endgroup\$

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.