Skip to content

Conversation

@kaufm
Copy link
Collaborator

@kaufm kaufm commented Aug 14, 2021

See my solution for the toRomanNumerals(num)function.

@wulfland wulfland self-requested a review August 14, 2021 08:43
@wulfland wulfland added the requirement A new requirement label Aug 14, 2021
Copy link
Owner

@wulfland wulfland left a comment

Choose a reason for hiding this comment

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

Great job 👏

@@ -1,5 +1,15 @@
function toRomanNumerals(num) {
return num;
var lookup = { M:1000,CM:900,D:500,CD:400,C:100,XC:90,L:50,XL:40,X:10,IX:9,V:5,IV:4,I:1 };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@wulfland, can you please help with the formatting here?

@wulfland wulfland force-pushed the main branch 8 times, most recently from c29cd2b to 7a67512 Compare October 27, 2021 09:15
Repository owner deleted a comment from kaufm Feb 16, 2022
}
}
return roman;

Copy link
Owner

Choose a reason for hiding this comment

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

Comment from mobile app 👍

Comment on lines +4 to +10
var i;
for ( i in lookup ) {
while ( num >= lookup[i] ) {
roman += i;
num -= lookup[i];
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
var i;
for ( i in lookup ) {
while ( num >= lookup[i] ) {
roman += i;
num -= lookup[i];
}
}
var item;
for ( i in lookup ) {
while ( num >= lookup[item] ) {
roman += item;
num -= lookup[item];
}
}

@wulfland wulfland linked an issue Feb 24, 2022 that may be closed by this pull request
3 tasks
@wulfland wulfland closed this Mar 21, 2023
@wulfland wulfland reopened this Mar 21, 2023
@wulfland wulfland closed this Mar 21, 2023
@wulfland wulfland reopened this Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requirement A new requirement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

💯 A new small requirement

2 participants