snippetrubyMinor
Convert cardinal abbreviations into full cardinal direction
Viewed 0 times
cardinalfullconvertintodirectionabbreviations
Problem
I'm still learning Ruby and I made the following function to convert
Code
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
"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
endTests
```
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).
-
-
When you have so many similar
First, with your code as reference mind, I'd write:
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).
-
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
endOn 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
endCode 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
endmodule 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
endContext
StackExchange Code Review Q#57495, answer score: 4
Revisions (0)
No revisions yet.