# Looking for feedback on JavaScript Roman Numeral Project

First off, it was a great challenge and my project went through many iterations before it worked, and one iteration to clean it up a bit.

I’m just looking for some feedback on it, I think I achieved a pretty good result, but I am wondering if there is a more efficient way than the one I finally settled on. And mostly just looking to hear other people’s solutions.

My final project looks like this:

``````function convertToRoman(num) {
let convert = num;
let result = [];
let roman =
{
1: "I", 4: "IV",  5: "V", 9: "IX",  10: "X",  40: "XL", 50: "L",
90: "XC", 100: "C", 400: "CD",  500: "D", 900: "CM",  1000: "M",
}
let cur;
while (convert.toString().length > 0 && convert != 0) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur]);
convert -= cur;
}
return result.join("")
}
console.log(convertToRoman(3999))
``````

I think this is pretty good, it also has good scalability, as you can simply add more roman numerals and it should work fine. I think its good tho, especially when you consider the version I had before I tried to make it smaller, you can see I realized that I was basically doing the exact same thing many times, when I could have just fit it all inside one loop instead, which is what I ended up doing.

``````function convertToRoman(num) {
let convert = num;
let result = [];
let roman = {
1: "I", 4: "IV",  5: "V", 9: "IX",  10: "X",  40: "XL", 50: "L",
90: "XC", 100: "C", 400: "CD",  500: "D", 900: "CM",  1000: "M",
}
let cur;
if (convert.toString().length == 4) {
while (convert.toString().length == 4) {
convert -= 1000;
result.push("M")
}
}
if (convert.toString().length == 3) {
while (convert.toString().length == 3) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur])
convert -= cur;
}
}
if (convert.toString().length == 2) {
while (convert.toString().length == 2) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur]);
convert-= cur;
}
}
if (convert.toString().length == 1 && convert != 0) {
while(convert.toString().length == 1 && convert !=0) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur]);
convert -= cur;
}
}
console.log(convert)
console.log(result)
return result.join("")
}

convertToRoman(3999);
``````

I think it’s safe to say the new version is at least much better than my old version lmao.

Your code has several sections of code that are almost duplicates except for one or two differences. The first two below are only different by the numbers `3` and `2` when comparing to `convert.toString().length`. The last one adds the `&& convert !=0` comparison. You should be able to make your code DRY (Do Not Repeat Yourself).
One way would be to put the repeated logic into a function that gets called.

``````    if (convert.toString().length == 3) {
while (convert.toString().length == 3) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur])
convert -= cur;
}
}
``````

and

``````    if (convert.toString().length == 2) {
while (convert.toString().length == 2) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur]);
convert-= cur;
}
}
``````

and

``````    if (convert.toString().length == 1 && convert != 0) {
while(convert.toString().length == 1 && convert !=0) {
for (const key in roman) {
if (convert / key >= 1) {
cur = key;
}
}
result.push(roman[cur]);
convert -= cur;
}
}
``````

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