Skip to content

Pr-10 #11

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.

Sign up for GitHub

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

Open
CheezItMan wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Pr-10 #11

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1953731
Roman Numeral conversion
CheezItMan Aug 15, 2019
71b855a
tests
CheezItMan Aug 15, 2019
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
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
class Fixnum

Comment thread
CheezItMan marked this conversation as resolved.
def roman
Copy link
Copy Markdown

@kdow kdow Aug 15, 2019

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?

Copy link
Copy Markdown

@Kimberly-Fasbender Kimberly-Fasbender Aug 15, 2019

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

@ranuseh ranuseh Aug 15, 2019

Choose a reason for hiding this comment

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

More descriptive variable names

Copy link
Copy Markdown

@shirley1chu shirley1chu Aug 15, 2019

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
Copy link
Copy Markdown

@paulentine paulentine Aug 15, 2019

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