# Roman Number Converter - Works but looking for Feedback on Idiomatic JS

My solution works but I’d like feedback on writing more compact or idiomatic code. Any other feedback is welcome as well.

``````//
function convertToRoman(num) {
var inum=num;
var rom =[];
while (inum >= 0) {
if (inum >= 1000) {rom.push("M"); inum-= 1000;}
else if (inum >= 900) {rom.push("CM"); inum -= 900;}
else if (inum >= 500) {rom.push("D"); inum -= 500;}
else if (inum >= 100) {rom.push("C"); inum -= 100;}
else if (inum >= 90) {rom.push("XC"); inum -= 90;}
else if (inum >= 50) {rom.push("L"); inum -= 50;}
else if (inum >= 40) {rom.push("XL"); inum -= 40;}
else if (inum >= 10) {rom.push("X"); inum -= 10;}
else if (inum == 9) {rom.push("IX"); inum -= 9;}
else if (inum >= 5) {rom.push("V"); inum -= 5;}
else if (inum == 4) {rom.push("IV"); inum -= 4;}
else if (inum < 4 && inum > 0) {rom.push("I"); inum-= 1;}
else if (inum <= 0) break;
else {console.log("something isn't right"); break;}
}

return rom.join("");
}``````

So your current code is quite readable and obvious in what it’s doing, that’s good! Somethings that jumped out at me though:

There’s so many “useless” checking whithin the `while` loop. Consider the value of `inum` within the loop after it’s less than say `500` - it has to check whether the number is greater than `1000`, then check if greater than `900`, then greater than `500`, and so on.

To get around this, you can remove the outer `while` loop, and replace the inner `if` statements with `while` loops themselves. e.g. `while(inum >= 1000) {do thing}`

As a side note, there doesn’t seem to be any reason to me to have another variable called `inum` - you can simply use `num` directly

Now there is another way to sort of remove `while`s entirely, at least hypothetically… If JS had a “string repeat” operator like in python etc we could use the division and remainder operations to skip doing a loop entirely, like in the following non-working pseudocode:

``````let repeatNumber = Math.floor(num / 1000);
num %= 1000;
rom.push(repeatNumber * "M");
``````

(where `*` is used to create a string of repeated “M”) This can actually be implemented in JS fairly efficiently, using a `O(log n)` method similarly to matrix power algorithms. If you fancied a short little challenge you could give that way a go.

Of course there’d be a form of loop “hidden” in the string repeat function, but yeah.

As an entirely alternative strategy one can use a “replacement table” and look up what to replace each value with - this is hardly cleaner though and it’s not clear to me whether it’d actually be faster

There is a string repeat in JS: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/repeat

I wouldn’t use an array, you can just concatenate to a string (initialize rom to ‘’ and do `rom += 'M'`, etc.). The last two clauses will never run, so you can remove those. And yeah, making a copy of num is rather pointless. It will not change any variable passed in, because `num` is already a copy.

Once you learn more about the array method `.reduce` you can write it a lot shorter. It will be a good exercise to revisit it at that time.