Skip to content

roman-numerals: add descriptions #707

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

Merged
merged 1 commit into from
Mar 11, 2017
Merged

roman-numerals: add descriptions #707

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions exercises/roman-numerals/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,74 +1,92 @@
{
"cases": [
{
"description": "1 is a single I",
"number" : 1,
"expected": "I"
},
{
"description": "2 is two I's",
"number" : 2,
"expected": "II"
},
{
"description": "3 is three I's",
"number" : 3,
"expected": "III"
},
{
"description": "4, being 5 - 1, is IV",
Copy link
Member Author

Choose a reason for hiding this comment

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

seems slightly illogical to refer to 4 as 5 - 1 before 5 has been introduced, but maybe someone thinks it's really important to use the natural ordering of numbers.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, I a bit torn on this one. Perhaps we should first do all of the individual letter case (I, V, X, etc.) and then move on to the edge cases? I think that would make more sense for this particular problem domain, even though it can seem a bit odd at first.

"number" : 4,
"expected": "IV"
},
{
"description": "5 is a single V",
"number" : 5,
"expected": "V"
},
{
"description": "6, being 5 + 1, is VI",
"number" : 6,
"expected": "VI"
},
{
"description": "9, being 10 - 1, is IX",
Copy link
Member Author

Choose a reason for hiding this comment

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

same: 9 as 10 - 1 before 10 introduced.

"number" : 9,
"expected": "IX"
},
{
"description": "20 is two X's",
"number" : 27,
"expected": "XXVII"
},
{
"description": "48 is not 50 - 2 but rather 40 + 8",
"number" : 48,
"expected": "XLVIII"
},
{
"description": "50 is a single L",
"number" : 59,
"expected": "LIX"
},
{
"description": "90, being 100 - 10, is XC",
Copy link
Member Author

Choose a reason for hiding this comment

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

same: 90 as 100 - 10 before 100 introduced.

"number" : 93,
"expected": "XCIII"
},
{
"description": "100 is a single C",
"number" : 141,
"expected": "CXLI"
},
{
"description": "60, being 50 + 10, is LX",
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided that the only new thing this case was testing was 60, but it's a bit of an illogical place to do that. It can be done earlier (near 50 and 90), without dealing with 100s. But I tried not to change tests in this PR.

"number" : 163,
"expected": "CLXIII"
},
{
"description": "400, being 500 - 100, is CD",
Copy link
Member Author

Choose a reason for hiding this comment

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

same: 400 as 500 - 100 before 500 introduced.

"number" : 402,
Copy link
Member Author

Choose a reason for hiding this comment

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

this tests a useful property noted in the README: empty places are skipped. Since the tens place is zero, we don't see an X or L in this answer. It might be useful to call this out. One might see this as distinct from 10 -> X because there are numbers on both sides of the zero.

Copy link
Member

Choose a reason for hiding this comment

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

+1 for calling out

"expected": "CDII"
},
{
"description": "500 is a single D",
"number" : 575,
"expected": "DLXXV"
},
{
"description": "900, being 1000 - 100, is CM",
Copy link
Member Author

Choose a reason for hiding this comment

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

same: 900 as 1000 - 100 before 1000 introduced.

"number" : 911,
"expected": "CMXI"
},
{
"description": "1000 is a single M",
"number" : 1024,
Copy link
Member Author

Choose a reason for hiding this comment

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

same, empty 100's place can be called out interestingly.

"expected": "MXXIV"
},
{
"description": "3000 is three M's",
"number" : 3000,
"expected": "MMM"
}
Expand Down