How ugly is this? really?

JavaScript
1

Tell us what’s happening:

I mean, it works, but just looking at some of the other answers out there, it feels like a fail.

Your code so far


function convertToRoman(num) {
   let romNumeralArray = [];
   while(num >= 1000){
       num = num - 1000;
       romNumeralArray.push("M");
   }
   while(num >= 900){
       num = num - 900;
       romNumeralArray.push("CM");
   }
   while(num >= 500){
       num = num - 500;
       romNumeralArray.push("D");
   }
   while(num >= 400){
       num = num - 400;
       romNumeralArray.push("CD");
   }
   while(num >= 100){
       num = num - 100;
       romNumeralArray.push("C");
   }
   while(num >= 90){
       num = num - 90;
       romNumeralArray.push("XC");
   }
   while(num >= 50){
       num = num - 50;
       romNumeralArray.push("L");
   }
   while(num >= 40){
       num = num - 40;
       romNumeralArray.push("XL");
   }
   while(num >= 10){
       num = num - 10;
       romNumeralArray.push("X");
   }
   while(num >= 9){
       num = num - 9;
       romNumeralArray.push("IX");
   }
   while(num >= 5){
       num = num - 5;
       romNumeralArray.push("V");
   }
   while(num >= 4){
       num = num - 4;
       romNumeralArray.push("IV");
   }
   while(num >= 1){
       num = num - 1;
       romNumeralArray.push("I");
   }
let string = romNumeralArray.join();
return string.replace(/[,]/g, "");
}

Your browser information:

User Agent is: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.88 Safari/537.36.

Challenge: Roman Numeral Converter

Link to the challenge:

2

it works, doesn’t it?

now try to think how to make it DRY (Don’t Repeat Yourself)

1 Like
3

It’s a perfectly good greedy algorithm, which is exactly what you use for this sort of problem. Now as @ilenia said, analyze the code and look for patterns you can move into functions.

So your next mission, should you choose to accept it, is to write a function that takes a number and returns two values (an array): the next piece of the roman numeral you’re building, and the new number subtracted by the amount of that roman numeral.

For example: nextNumeral(432) should return ["CD", 132].

Now write your roman numeral converter using this function. BTW, there are other ways to break down this converter into functions, but stick around for Part 2 and I’ll show you why this function is written this way.

1 Like
4

Thanks, I tried! Well… I got part way through it and then ended up coming up with this:

function convertToRoman(num) {
 let newArray = [];
 let numeralNumber = [1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1];
 let numeralLetter = ["M", "CM", "D", "CD", "C", "XC", "L", "XL", "X", "IX", "V", "IV", "I"];
 let i;
for(i=0; i<13; i++){
  while(num >= numeralNumber[i]){
       num = num - numeralNumber[i];
       newArray.push(numeralLetter[i]);
   }  
} 
 return newArray.join().replace(/[,]/g, "");;
}

I am much happier with this one but… I feel I may have missed your point :sweat_smile:

5

My larger point got sidetracked by work, sorry :slight_smile: Anyway, the idea of having a function like number -> [string, number] is a functional programming technique, and I was trying to use you as a guinea pig to demonstrate it.

Unfortunately once I worked through it in my head, the functional answer wasn’t as clear as I’d imagined, at least not the way I gave that function (specifically it would need an “unfold” operation), so it’s probably the best that you didn’t chase me down that rabbit hole :crazy_face:

The algorithm you have is very good, and efficient at that. I’m normally concerned about keeping two arrays in sync like that, and would usually zip them instead, i.e. have [[1000, "M"], [900, "CM"] ...etc]. But they aren’t changing the roman number system anytime soon, so once the algorithm works once, those lists wouldn’t need to change :slight_smile:

6

I was curious if having the separate arrays was wrong… I couldn’t figure out why though.

I guess the way you explained it makes sense. If someone wanted to update these in the future and delete or add something then they’d have to access both and get the sync right. However if they were paired in an object or something then this eliminates that problem?