3
\$\begingroup\$

Is this an efficient way to implement the Roman Numeral Converter ?

So first, I put all the digits of the parameter number into an array.

Then I loop through this array, and multiply a 10 power based number to the current digit.

I use the resulted number in multiple statements, to compare it with special cases or numbers bigger or smaller than those cases.

For each if statement, when it is true, I concatenate a corresponding roman numeral to the whole.

At the end I return the final Roman Numeral as a string.

function convertToRoman(num) {
  
  var digitArr = []; // 2

  while(num !== 0){
    digitArr.push(num % 10); // 2 
    num = Math.floor(num / 10); // num = 0
  }
  
  
  var len = digitArr.length;
  var romanNumber = "";
  var numb;
  var number;
  

  for(var i = len-1;i >= 0;i--){ // 1 0
    number = Math.pow(10, i) * digitArr[i]; // 3

    if(number >= 1000){
      for(var j = 0;j < digitArr[i];j++)romanNumber += "M";
    }else if(number === 900){
      romanNumber += "CM";
    }else if(number > 500){
      numb = number / 100 - 5;
        romanNumber += "D";
        for(var k = 0;k < numb;k++)romanNumber += "C";
    }else if(number === 500){
      romanNumber += "D";
    }else if(number === 400){
      romanNumber += "CD";
    }else if(number < 400 && number >= 100){
      numb = number / 100;
        for(var l = 0;l < numb;l++)romanNumber += "C";
    }else if(number === 90){
      romanNumber += "XC";
    }else if(number >= 60 && number < 90){
      numb = number / 10 - 5;
      romanNumber += "L";
      for(var z = 0;z < numb;z++)romanNumber += "X";
    }else if(number === 50){
      romanNumber += "L";
    }else if(number === 40){
      romanNumber += "XL";
    }else if(number < 40 && number >= 10){
      numb = number / 10;
      for(var x = 0;x < numb;x++)romanNumber += "X";
    }else if(number === 9){
      romanNumber += "IX";
    }else if(number < 9 && number >= 6){
      numb = number - 5;
      romanNumber += "V";
      for(var y = 0;y < numb;y++)romanNumber += "I";
    }else if(number === 5){
      romanNumber += "V";
    }else if(number === 4){
      romanNumber += "IV";
    }else if(number < 4){
      for(var p = 0;p < number;p++){
        romanNumber += "I";
      }
    }
  }

  return romanNumber;
}

\$\endgroup\$
  • \$\begingroup\$ See stackoverflow.com/questions/9083037/… for a number of possible implementations, most of which appear more elegant than what you propose. \$\endgroup\$ – Mike Brant Jun 19 '17 at 21:43
  • \$\begingroup\$ Do not forget that 5,000 is not MMMMM but V̅ (same for 10,000 and so on...) \$\endgroup\$ – Adriano Repetti Jun 20 '17 at 11:40
2
\$\begingroup\$

See link above in comments for some more elegant solutions. So I am not going to focus on algorithm, which I think can certainly be improved.

Some thoughts on your code itself:

  • Comments don't seem to make sense or add value.
  • No need for all the if-else stuff. Would be much clearer with just if conditions and breaks, IMO.
  • I don't understand why you build digitArray the way you do. How about something like num.toString().split('').map(function(t){return parseInt(t)}) or Array.from(num.toString()).map(Number) (ES6)?
  • Your spacing around conditional and flow-control structures (or really, lack thereof) makes your code hard to read.
  • Indentation is inconsistent.
  • Seems to be odd use of vertical whitespace.
\$\endgroup\$
  • \$\begingroup\$ It should be noted that you use .map((t) => parseInt(t)) instead of just .map(parseInt), and there's a good reason for that. \$\endgroup\$ – Daerdemandt Jun 21 '17 at 0:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.