# Roman Numerals Converter - Is this solution "clean"?

Hey everyone.

I’ve been doing some JavaScript recently, and i thought i’d shoot for the projects to see where i’m at in terms of knowledge.
Therefore i’m trying to complete the projects with “clean” solutions, but since i don’t really actually know what makes some solutions better than others (apart from time complexity), i’d like some input on this one. I was really happy with it because it’s very short and readable, but i know that doesn’t necessarily mean GOOD code. I just thought this one was good because it doesn’t need as many comparisons and conditions as using the “number approach”
So here it is ,

``````function convertToRoman(num)
{
let romans =
[
["I", "II", "III", "IV", "V", "VI", "VII", "VIII", "IX"],
["X", "XX", "XXX", "XL", "L", "LX", "LXX", "LXXX","XC"],
["C", "CC", "CCC", "CD", "D", "DC", "DCC", "DCCC", "CM"],
["M"],
];

let split = num.toString().split('').reverse();

return split.map((n, i) => i === 3 ? `\${romans[i][0].repeat(parseInt(n))}`: n > 0 ? romans[i][parseInt(n) - 1] : '').reverse().join('');

}

``````

Thanks in advance for reading, i welcome any and all input !

I’d git rid of the double reverse, personally. Also, why make the thousands a special case? You can only go up to 3999 with traditional Roman numerals, so just use the same logic as above instead of making a special case.

Yeah i was thinking that the double reverse isn’t great. Also very good point, idk why i went for that with thousands, will get rid of it now. Thanks man.

EDIT : Although for the double reverse, it kinda forces me to either check the length of split or do some more operations with indexes in the logic ? Genuinely curious to know why double reverse wouldn’t be more efficient/readable in this case where we know the size of the obtained arrays to be minuscule.

It’s technically not a performance issue given the input sizes, it’s just a little bit odd imho. Why do extra work that isn’t needed? The length is known and constant.

``````return arabic
.split('')
.map((n, i) => ROMANS[arabic.length - 1 - i][n])
.join('');
``````

I’d pair this with a few tweaks like moving the `ROMANS` variable to be a global constant and changing the contents a little bit.

Will do, thanks a bunch. Have a good one

1 Like

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.