JavaScript Algorithms and Data Structures Projects - Roman Numeral Converter

Code Feedback
#1

Does anyone have any tips for a more optimal solution?
any response is appreciated

Your code so far

function convertToRoman(num) {
 let romanNumeral = [{M:1000}, {CM:900}, {D:500}, {CD:400}, {C:100}, {XC:90}, {L:50}, {XL:40}, {X:10}, {IX:9}, {V:5}, {IV:4}, {I:1}];
let finalStr = [];
let newNum = num;

 while (newNum > 0) {
   for (let i = 0; i < romanNumeral.length; i++) {
     if (newNum >= Object.values(romanNumeral[i])) {
       finalStr.push(Object.keys((romanNumeral[i])));
       newNum -= parseInt(Object.values(romanNumeral[i]),10);
       
       i = -1;
       
       
       
       
       
       
       
       
       
       
       
       
       
       
     }
     }
   }
   
   return finalStr.join("");
 }


console.log(convertToRoman(2014));

Your browser information:

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/108.0.0.0 Safari/537.36

Challenge: JavaScript Algorithms and Data Structures Projects - Roman Numeral Converter

Link to the challenge:

#2

There are probably a few changes you could make to tighten this up a little.

  • Do you need to store finalStr as an array? You are just using it to build a string. Can you just build a string instead without the array? This isn’t going to make a lick of difference as far as how fast your function runs. I just think it’s one less conversion you have to deal with when reading the code.
  • What is the purpose of newNum? Is a number passed in to a function by value or reference?
  • i = -1 seems weird to me and it’s not exactly evident what it does on first glance. If you want to break out of a loop then break out of it.
  • I don’t think you need to use parseInt. The values you are storing in romanNumeral are numbers so Object.values should return an array of numbers.
  • You can potentially reduce the amount for loop iterations for certain numbers if you don’t keep starting at 0. For example, if you know that the number remaining is less than 100, do you really need to check if the number is greater than all the values in romanNumeral that are greater than/equal to 100? In other words, allow i to retain its value when another iteration of the for loop is started. Granted, this is a micro optimization and won’t make any difference in how fast this runs. But knowing that there are unnecessary loops running will keep you up at night :slightly_smiling_face: