Roman Numeral Converter Review

Code Feedback
1

Hi All!

I finally got this one to work! It’s pretty bulky though lol. Could you provide some feedback as to how I could tidy this up without really giving me the answer? I’d like to refactor this and improve upon it, but really just be pointed in the right direction.


function convertToRoman(num) {
 
  var numKey = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 20, 30, 40, 50, 60, 70, 80, 90, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1000, 2000, 3000, 4000];
  var romanKey = ['I', 'II', 'III', 'IV', 'V', 'VI', 'VII', 'VIII', 'IX', 'X', 'XX', 'XXX', 'XL', 'L', 'LX', 'LXX', 'LXXX', 'XC', 'C', 'CC', 'CCC', 'CD', 'D', 'DC', 'DCC', 'DCCC', 'CM', 'M', 'MM', 'MMM'];
  
  var convertNum = String(num);
  var number = convertNum.split('');
  var final = [];
  
  if (number.length === 1) {
    
    var one = number[0];
    one = numKey.indexOf(Number(one));
    one = romanKey[one];
    final.push(one);
    
  } else if (number.length === 2) {
    
    var ten = number[0] + '0';
    ten = numKey.indexOf(Number(ten));
    ten = romanKey[ten];
    final.push(ten);
    
    var tenOne = number[1];
    tenOne = numKey.indexOf(Number(tenOne));
    tenOne = romanKey[tenOne];
    final.push(tenOne);
    
  } else if (number.length === 3) {
    
    var hundo = number[0] + '0' + '0';
    hundo = numKey.indexOf(Number(hundo));
    hundo = romanKey[hundo];
    final.push(hundo);
    
    var hundoTen = number[1]  + '0';
    hundoTen = numKey.indexOf(Number(hundoTen));
    hundoTen = romanKey[hundoTen];
    final.push(hundoTen);    
    
    var hundoTenOne = number[2];
    hundoTenOne = numKey.indexOf(Number(hundoTenOne));
    hundoTenOne = romanKey[hundoTenOne];
    final.push(hundoTenOne); 
    
  } else if (number.length === 4) {
    
    var thou = number[0] + '0' + '0' + '0';
    thou = numKey.indexOf(Number(thou));
    thou = romanKey[thou];
    final.push(thou);
 
    var thouHundo = number[1] + '0' + '0';
    thouHundo = numKey.indexOf(Number(thouHundo));
    thouHundo = romanKey[thouHundo];
    final.push(thouHundo);    
    
    var thouHundoTen = number[2] + '0';
    thouHundoTen = numKey.indexOf(Number(thouHundoTen));
    thouHundoTen = romanKey[thouHundoTen];
    final.push(thouHundoTen);  

    var thouHundoTenOne = number[3];
    thouHundoTenOne = numKey.indexOf(Number(thouHundoTenOne));
    thouHundoTenOne = romanKey[thouHundoTenOne];
    final.push(thouHundoTenOne);      
    
  }
    
  return final.join('');

}

convertToRoman(36);

I feel like I should be using a loop for each iteration of the number especially in the thousands since I’m doing number 0,1,2,3. I’m just really confused about how to make it a number that would be able to be found in my initial arrays, but maybe I’m just thinking about it wrong in general.

Thanks,
Seth

1 Like
2

I just finished this one myself - it was tough. We have similar ideas. I used an object instead of two arrays for the keys, and used reusable for-loops since the same rules apply for each position but with different symbols, and unshift instead of push.
Here it is:

Summary 
function convertToRomanNum(num) {
  /* Code is valid up to 399,999 because I did not include the symbol 
     for 500,000 required for 400,000+. Note that Ṽ, Ẋ, Ḹ, Č, were used 
    because I couldn't figure out how to apply overlines to letters. 
    "I" is equal to 1
    "V" is equal to 5
    "X" is equal to 10
    "L" is equal to 50
    "C" is equal to 100
    "D" is equal to 500
    "M" is equal to 1,000
    "Ṽ" is equal to 5,000
    "Ẋ" is equal to 10,000
    "Ḹ" is equal to 50,000
    "Č" is equal to 100,000
  */
  
  const characters = {
    1: "I",
    2: "V",
    3: "X",
    4: "L",
    5: "C",
    6: "D",
    7: "M",
    8: "Ṽ",
    9: "Ẋ",
    10: "Ḹ",
    11: "Č"
  };
  
  var roman = [];
  var numArr = num.toString().split('');
  
  const digits = {
    1: numArr[numArr.length - 1],
    3: numArr[numArr.length - 2],
    5: numArr[numArr.length - 3],
    7: numArr[numArr.length - 4],
    9: numArr[numArr.length - 5],
    11: numArr[numArr.length - 6]
  };
  
  for (let i = 1; i < numArr.length * 2; i = i + 2) {
    
    if (digits[i] > 0 && digits[i] <= 3) {
      for (let j = 0; j < digits[i]; j++) {
      roman.unshift(characters[i]);
      }
    }
  
    else if (digits[i] == 4) {
      roman.unshift(characters[i + 1]);
      roman.unshift(characters[i]);
    }
  
    else if (digits[i] == 5) {
      roman.unshift(characters[i+1]);
    }
  
    else if (digits[i] >= 6 && digits[i] <= 8) {
      for (let j = 5; j < digits[i]; j++) {
        roman.unshift(characters[i]);
      }
        roman.unshift(characters[i + 1]);
    }
  
    else if (digits[i] == 9) {
      roman.unshift(characters[i + 2]);
      roman.unshift(characters[i]);
    }
  
    else ;
    
  } //closing bracket for outer condition
  
  var result = roman.join('');
 
  return result;
}

convertToRomanNum(1973);