# Roman Converter feedback

I can’t tell if my solutions are ingenious or unnecessarily complicated. The logic behind my code is to break up any number <= 4000 into components, so 1234 would become `[1000, 200, 30, 4]` and then simply find their equivalents from the tables by using the indices of the Arabic numerals since they correspond to the indices of the Roman numerals. Here’s the code with explanations of each step. Any feedback is appreciated.

``````function convertToRoman(n) {
let romanTillTen = ['I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX', 'X'];
let arTillTen = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
let romanTillHund = ['XX', 'XXX', 'XL', 'L', 'LX', 'LXX', 'LXXX', 'XC', 'C'];
let arTillHund = [20, 30, 40, 50, 60, 70, 80, 90, 100];
let romanTillMil = ['CC', 'CCC', 'CD', 'D', 'DC', 'DCC', 'DCCC', 'CM', 'M'];
let arTillMil = [200, 300, 400, 500, 600, 700, 800, 900, 1000];
let romanTill4k = ['MM', 'MMM', 'MMMM'];
let arTill4k = [2000, 3000, 4000];
let numOfzeros = n.toString().length; // find length of n by turning it to a string
let zeros = [1]; // an array to store the inital value to divide n by to get remainders
let rem = [n]; // an array storing the remainders, which will be used to find components
let components = []; // an array to store components, e.g 3999 = [3000, 900, 99]
let final = []; // the final array with corresponding roman numbers
for (let i = 0; i < numOfzeros - 1; i++) { // push numOfzeros - 1 amount of 0's into zeros, so 3999 would result in [1, 0, 0, 0]
zeros.push(0)
}
zeros = Number(zeros.join('')) // turn zeros array into a single number, so [1, 0, 0, 0] becomes 1000
while (zeros >= 10) { // rem.push(3999 % 1000), (999 % 100), (99 % 10)
rem.push(n % zeros)
zeros /= 10
}
for (let i = 0; i < rem.length - 1; i++) { // [3999, 999, 99, 9] becomes [3000, 900, 90], last element not included to avoid NaN
components.push(rem[i] - rem[i + 1])
}
components.push(rem[rem.length - 1]); // push last number of rem into components since it was not in the previous loop
components = components.filter(a => a > 0) // remove any 0 values if any, now components is [3000, 900, 90, 9]
for (let num of components) { // since the indices of Arabic numerals correspond to their Roman equivalents, get the Roman numerals at given indices
if (num <= 10) {
final.push(romanTillTen[arTillTen.indexOf(num)])
} else if (num > 10 && num <= 100) {
final.push(romanTillHund[arTillHund.indexOf(num)])
} else if (num > 100 && num <= 1000) {
final.push(romanTillMil[arTillMil.indexOf(num)])
} else if (num > 1000 && num <= 4000) {
final.push(romanTill4k[arTill4k.indexOf(num)])
}
}
return final.join('')
}

console.log(convertToRoman(3999));
``````

HI @Montin !

Well it is unique.
I have never seen this approach before.
But the main thing is that you solved it.

A good next step, would be to look at other solutions and see the different ways people chose to solve this.
This is a pretty common algorithm, so it will show up all over the internet.

But one thing, that would be good to start working on are your variable names.
You want to work on creating meaningful variable names where all developers can understand what you mean.

For example here

`````` let rem
``````

rem is not a good variable name.
Maybe something like `arrOfRemainders` would be better.

Another example would be `final`

What is final supposed to be?
When I look at your comment, it is supposed to be the final output array.

You can change it to something like `finalRomanNumeralOutputArr`.
Or something like that.

Having clear variable name will cut down on the amount of comments you are using to try to explain your code.

Naming variables and functions is hard and is a skill to work on.
But it will be an important skill to learn when you start working on production level codebases at work.

Hope that helps!

2 Likes

Thank you so much for your feedback! Yes, sometimes I get lazy with variable names and try to keep them as short as possible but I know that affects the readability, I’ll keep that in mind from now on

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