HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Number to Roman numerals

Submitted by: @import:stackexchange-codereview··
0
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

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

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.