patternjavascriptMinor
Number to Roman numerals
Viewed 0 times
numberromannumerals
Problem
I've been working on numerals conversion lately. Now the JavaScript course I'm following asked me to do something similar for Roman numerals.
To keep things fresh and given the amount of test-cases provided I decided to write this one Test-Driven Development style. That is, for as much as I understand it.
First I check whether it's dividable by a full numeral (M, C, etc.). After that I have to reverse the string because the numerals were added in reverse order. The next step is to translate all combinations into their correct form using regex and a translation map. The functions were written in that order and fixing the problem the last part caused.
So far it looks very messy. Is this how one is supposed to solve problems TDD style? There is definitely a lot of code duplication going on, could this be fixed with another map?
Challenge
Convert the given number into a Roman numeral.
All Roman numerals answers should be provided in upper-case.
Test cases
Code
```
function convert(num) {
roman = "";
for (var i = 0; i < num; i++) {
//MDCLXVI
if (!(num % 1000)) { roman += "M"; num -= 1000; }
else if (!(num % 500)) { roman += "D"; num -= 500; }
else if (!(num % 100)) { roman += "C"; num -= 100; }
else if (!(num % 50)) { roman += "L"; num -= 50; }
else if (!(num % 10)) { roman += "X"; num -= 10; }
else if (!(num % 5)) { roman += "V"; num -= 5; }
else if (!(num % 1)){ roman += "I"; num -= 1; }
}
roman = roman.split(''
To keep things fresh and given the amount of test-cases provided I decided to write this one Test-Driven Development style. That is, for as much as I understand it.
First I check whether it's dividable by a full numeral (M, C, etc.). After that I have to reverse the string because the numerals were added in reverse order. The next step is to translate all combinations into their correct form using regex and a translation map. The functions were written in that order and fixing the problem the last part caused.
So far it looks very messy. Is this how one is supposed to solve problems TDD style? There is definitely a lot of code duplication going on, could this be fixed with another map?
Challenge
Convert the given number into a Roman numeral.
All Roman numerals answers should be provided in upper-case.
Test cases
convert(5) // "V".
convert(9) // "IX".
convert(12) // "XII".
convert(16) // "XVI".
convert(29) // "XXIX".
convert(44) // "XLIV".
convert(45) // "XLV"
convert(68) // "LXVIII"
convert(83) // "LXXXIII"
convert(97) // "XCVII"
convert(99) // "XCIX"
convert(500) // "D"
convert(501) // "DI"
convert(649) // "DCXLIX"
convert(798) // "DCCXCVIII"
convert(891) // "DCCCXCI"
convert(1000) // "M"
convert(1004) // "MIV"
convert(1006) // "MVI"
convert(1023) // "MXXIII"
convert(2014) // "MMXIV"
convert(3999) // "MMMCMXCIX"Code
```
function convert(num) {
roman = "";
for (var i = 0; i < num; i++) {
//MDCLXVI
if (!(num % 1000)) { roman += "M"; num -= 1000; }
else if (!(num % 500)) { roman += "D"; num -= 500; }
else if (!(num % 100)) { roman += "C"; num -= 100; }
else if (!(num % 50)) { roman += "L"; num -= 50; }
else if (!(num % 10)) { roman += "X"; num -= 10; }
else if (!(num % 5)) { roman += "V"; num -= 5; }
else if (!(num % 1)){ roman += "I"; num -= 1; }
}
roman = roman.split(''
Solution
Roman letter to value map
You should be using a map here to store the roman numeral values and their corresponding characters:
With this, you can easily add on more letters if you ever felt like expanding it. Now, here is what your code would look like there:
Then, to OOP this code up, you could move this function and the new map to an object to keep things together:
Then, the map won't be created every time the function is called; it will be created and referenced once.
Moar regexes... moar!
In this loop:
Every iteration, you are creating a regex for every entry in the map. However, every time this function is called, the same regexes are created because that map is the same every time.
To speed things up, try creating the regex once and keeping it in the map that is also in this
Your code will now be faster because a new series of regexes doesn't have to be created every function call.
if (!(num % 1000)) { roman += "M"; num -= 1000; }
else if (!(num % 500)) { roman += "D"; num -= 500; }
else if (!(num % 100)) { roman += "C"; num -= 100; }
else if (!(num % 50)) { roman += "L"; num -= 50; }
else if (!(num % 10)) { roman += "X"; num -= 10; }
else if (!(num % 5)) { roman += "V"; num -= 5; }
else if (!(num % 1)){ roman += "I"; num -= 1; }You should be using a map here to store the roman numeral values and their corresponding characters:
var romanCharacterMap = {
"M": 1000,
"D": 500,
...
}With this, you can easily add on more letters if you ever felt like expanding it. Now, here is what your code would look like there:
while (num > 0) {
for(var romanCharacter in romanCharacterMap) {
var value = romanCharacterMap[romanCharacter];
if(!(num % value)) {
roman += romanCharacter;
num -= value;
break;
}
}
}Then, to OOP this code up, you could move this function and the new map to an object to keep things together:
var RomanNumberalConverter = {
...
}Then, the map won't be created every time the function is called; it will be created and referenced once.
Moar regexes... moar!
In this loop:
for (var i in translationMap) {
roman = roman.replace(new RegExp(i,'g'), translationMap[i]);
}Every iteration, you are creating a regex for every entry in the map. However, every time this function is called, the same regexes are created because that map is the same every time.
To speed things up, try creating the regex once and keeping it in the map that is also in this
RomanNumberConverter object:translationMap: {
DCCCC: {
replace: "CM",
regex: /DCCCC/g
},
...
}Your code will now be faster because a new series of regexes doesn't have to be created every function call.
Code Snippets
if (!(num % 1000)) { roman += "M"; num -= 1000; }
else if (!(num % 500)) { roman += "D"; num -= 500; }
else if (!(num % 100)) { roman += "C"; num -= 100; }
else if (!(num % 50)) { roman += "L"; num -= 50; }
else if (!(num % 10)) { roman += "X"; num -= 10; }
else if (!(num % 5)) { roman += "V"; num -= 5; }
else if (!(num % 1)){ roman += "I"; num -= 1; }var romanCharacterMap = {
"M": 1000,
"D": 500,
...
}while (num > 0) {
for(var romanCharacter in romanCharacterMap) {
var value = romanCharacterMap[romanCharacter];
if(!(num % value)) {
roman += romanCharacter;
num -= value;
break;
}
}
}var RomanNumberalConverter = {
...
}for (var i in translationMap) {
roman = roman.replace(new RegExp(i,'g'), translationMap[i]);
}Context
StackExchange Code Review Q#117635, answer score: 8
Revisions (0)
No revisions yet.