HiveBrain v1.2.0
Get Started
← Back to all entries
snippetrubyMinor

Convert cardinal abbreviations into full cardinal direction

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cardinalfullconvertintodirectionabbreviations

Problem

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(u

Solution

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

Code Snippets

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
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

Context

StackExchange Code Review Q#57495, answer score: 4

Revisions (0)

No revisions yet.