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

JavaScript
1

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("");
 }
2

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 whiles 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

3

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

4

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.