Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr-10 #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions roman.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
class Fixnum

def roman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this is in a method?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we think about making this an instance variable vs. a function?

{
1000 => "M",
900 => "CM",
500 => "D",
400 => "CD",
100 => "C",
90 => "XC",
50 => "L",
40 => "XL",
10 => "X",
9 => "IX",
5 => "V",
4 => "IV",
1 => "I"
}
end

def to_roman
n = self
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More descriptive variable names

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would benefit to leave comment that n is an instance of Fixnum, which gets converted to roman via to_roman

result = ""
roman.each do |num, letter|
result << letter * (n / num)
n = n % num

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be slightly shortened with n %= num

end
return result
end
end
95 changes: 95 additions & 0 deletions roman_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
require 'minitest/autorun'
require_relative 'roman_numerals'

# Common test data version: 1.0.0 070e8d5

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add edge cases?

class RomanNumeralsTest < Minitest::Test
def test_1
# skip
assert_equal 'I', 1.to_roman
end

def test_2
skip

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to be skipping these tests? We probably want to comment these out.

assert_equal 'II', 2.to_roman
end

def test_3
skip
assert_equal 'III', 3.to_roman
end

def test_4
skip
assert_equal 'IV', 4.to_roman
end

def test_5
skip
assert_equal 'V', 5.to_roman
end

def test_6
skip
assert_equal 'VI', 6.to_roman
end

def test_9
skip
assert_equal 'IX', 9.to_roman
end

def test_27
skip
assert_equal 'XXVII', 27.to_roman
end

def test_48
skip
assert_equal 'XLVIII', 48.to_roman
end

def test_59
skip
assert_equal 'LIX', 59.to_roman
end

def test_93
skip
assert_equal 'XCIII', 93.to_roman
end

def test_141
skip
assert_equal 'CXLI', 141.to_roman
end

def test_163
skip
assert_equal 'CLXIII', 163.to_roman
end

def test_402
skip
assert_equal 'CDII', 402.to_roman
end

def test_575
skip
assert_equal 'DLXXV', 575.to_roman
end

def test_911
skip
assert_equal 'CMXI', 911.to_roman
end

def test_1024
skip
assert_equal 'MXXIV', 1024.to_roman
end

def test_3000
skip
assert_equal 'MMM', 3000.to_roman
end
end