Roman Numerals Converter - Is this solution "clean"?

JavaScript
1

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 !

2

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.

3

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.

4

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.

5

Will do, thanks a bunch. Have a good one

1 Like
6

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