Roman Number Converter

Code Feedback
1

Hi folks,

This is my solution for the Roman Number Converter.

// @krisboyy's solution to the problem Roman Numeral Converter
// https://www.freecodecamp.org/learn/javascript-algorithms-and-data-structures/javascript-algorithms-and-data-structures-projects/roman-numeral-converter

function convertToRoman(num) {
  const lookup = {
    1000: 'M',
    900: 'CM',
    500: 'D',
    400: 'CD',
    100: 'C',
    90: 'XC',
    50: 'L',
    40: 'XL',
    10: 'X',
    9: 'IX',
    5: 'V',
    4: 'IV',
    1: 'I',
  }
  
  // Converting the number to a string, splitting the string into an array
  // and reversing the array to work from units place
  let numArr = String(num).split('').reverse();
  console.log(numArr)

  let romanArr = []
  // Loop through the digits of the number
  for(let i=0; i<numArr.length; i++){
    
    // value is the value of the digit we are considering now
    // value = digit * 10^i
    let value = numArr[i] * Math.pow(10, i)
    
    // If number is not in the lookup table, then it either lies in the range
    // (1*10^i, 4*10^i) (exclusive) or (5*10^i, 9*10^i) (exclusive)
    if(lookup[value] == undefined){

      // Check if the number lies in the range (1*10^i, 4*10^i)
      if(value<(4*Math.pow(10,i))){

        // temp is used to control how many times the roman numeral
        // should be repeated to make up the value.
        
        // Take an example of converting the number 23
        // The unit's place contains 3. So roman 'I' has to repeat
        // three times to make the value of 3.
        // The ten's place contains 2. So roman 'X' has to repeat
        // two times to make the value of 20.
        let temp = numArr[i];

        // repeatingRoman represents the roman numeral that will repeat to
        // make the value
        let repeatingRoman = lookup[Math.pow(10,i)];

        // while loop to repeat and store the corresponding roman numerals
        while(temp>0){
          // Since we have reversed our number in the beginning,
          // we are unshifting into the romanArr (pushing from the front).
          romanArr.unshift(repeatingRoman);
          temp--;
        }


      } else{
        // The number lies in the range (5*10^i, 9*10^i)

        let temp = numArr[i];

        // Here, the roman representation will contain a representation of
        // 5*10^i followed by some repeating numeral
        temp = temp - 5;
        // romanFive identifies the corresponding representation of 5*10^i
        let romanFive = lookup[5*Math.pow(10,i)]

        let repeatingRoman = lookup[Math.pow(10,i)]
        while(temp>0){
          romanArr.unshift(repeatingRoman);
          temp--
        }
        // we unshift the representation of 5*10^i after unshifting the
        // repeating numerals
        romanArr.unshift(romanFive)
      }
    } else {
      // if the number is found in the lookup table, directly unshift it
      // into the romanArr.
      romanArr.unshift(lookup[value]);
    }
  }
  // Join the romanArr
 return romanArr.join('');
}

console.log(convertToRoman(12))
console.log(convertToRoman(83))
console.log(convertToRoman(400))
console.log(convertToRoman(1023))
console.log(convertToRoman(3999))

The output:

[ '2', '1' ]
XII
[ '3', '8' ]
LXXXIII
[ '0', '0', '4' ]
CD
[ '3', '2', '0', '1' ]
MXXIII
[ '9', '9', '9', '3' ]
MMMCMXCIX

I couldn’t figure out the solution to the problem, but I wanted to develop an algorithm that doesn’t rely on hard-coded values. I was discouraged after a few tries and that’s when I saw the solutions many of you guys posted. The attitude that you guys showed to get the results you wanted really motivated me to sit down and figure out the algorithm that I had in my mind. And I am very happy that I was able to make this!

Please let me know if you find any suggestions for improvement.

1 Like
2
  • Sticking out the most are comments, most of the time they are repeating what’s already in code. Generally comments should focus on why something is done, not what. For example:
        // Check if the number lies in the range (1*10^i, 4*10^i)
    if(value<(4*Math.pow(10,i))){
    While comment describing what the check does is obsolete, it could explain why is it relevant that value is below that number or not.
  • Variable names - this is partially related to point above - when variable has clear name, there’s no need to add comment explaining what that variable is. I’m looking especially at the temp variable.
  • Nesting. Take a look at:
    for () {
  if () {
    if () {
      while () {
      }
    } else {
      while () {
      }
    }
  } else {
  }
}
    These are main parts of the code above. They are slowly getting more and more nested, making it harder to follow. Since this is everything in a loop, at least one level of the if/else could be removed with ease - at the end of if contents, add continue, then dedent code in else and remove the else keyword its brackets {}. Because at the end of if will be continue, the code below it will not be executed at the same time as if contents, and rest can be dedented. Usually this has best results when contents of if is shorter than the rest of code, if that’s not a case, the if condition can always be flipped to make it so.
  • Repeated if/else contents. If most of these are repeated, there could be a way to reduce the repetition by extracting matching parts out of the if/else.
        if(value<(4*Math.pow(10,i))){

      let temp = numArr[i];
      let repeatingRoman = lookup[Math.pow(10,i)];

      while(temp>0){
        romanArr.unshift(repeatingRoman);
        temp--;
      }
    } else{
      let temp = numArr[i];

      temp = temp - 5;
      let romanFive = lookup[5*Math.pow(10,i)]

      let repeatingRoman = lookup[Math.pow(10,i)]

      while(temp>0){
        romanArr.unshift(repeatingRoman);
        temp--
      }
      romanArr.unshift(romanFive)
    }
  • Personally, instead of unshift, I’d use push and the reverse array at the end. I’m always needing a moment when seeing shift/unshift, to remind myself which one does what.

Otherwise :+1: