1
\$\begingroup\$

I've created a function that takes an integer greater than zero as an input and converts it into its corresponding Roman numeral. If you're not familiar with Roman numerals, you can read about it on Wikipedia. I've tested this code with some examples, so it should be working correctly.

I'd be interested in hearing about any general improvements I could make on the code. Nitpicky details are fine as well; whatever you have to say I'm willing to hear, but I hope your answer is more substantial than that. I suppose my main concern is whether this is sufficiently efficient or if there is a way I could improve on that. Also, do you think I've used the correct data structure in this situation? I can't think of a better data structure for what I wanted to do, so that is why I chose arrays.

function arabicToRoman(num) {
    var roman = ["I", "IV", "V", "IX", "X", "XL", "L", "XC", "C", "CD", "D", "CM", "M"];
    var arabic = [1, 4, 5, 9, 10, 40, 50, 90, 100, 400, 500, 900, 1000];
    var result = "";

    while(num >= 4000) {
        result += "M";
        num -= 1000;
    }

    for(var i = arabic.length - 1; num > 0; i--) {
        while(num >= arabic[i]) {
            result += roman[i];
            num -= arabic[i];
        }
    }

    return result;
}
\$\endgroup\$
  • \$\begingroup\$ I think this is an excellent solution, probably the fastest: 1 million runs of arabicToRoman(785) in 200ms. \$\endgroup\$ – wOxxOm Sep 11 '16 at 10:56
3
\$\begingroup\$

First, you can easily throw away semicolons ";".

Second, if you want shorter code, you can do like this:

function arabicToRoman(num) {
  var roman = {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}
  var result = ''

  for (var i in roman) {
    while (num >= roman[i]) {
      result += i
      num -= roman[i]
    }
  }
  return result
}

But, it's also almost ten times slower, so it's up to you - do you need compact code, or performance?

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review and nice first answer, enjoy your stay! \$\endgroup\$ – ferada Sep 11 '16 at 14:02
  • \$\begingroup\$ I'm looking for performance, not minification. \$\endgroup\$ – kamoroso94 Sep 11 '16 at 18:45
  • \$\begingroup\$ @kamoroso94, if you looking for performance - then add memoization, for example. \$\endgroup\$ – aring Sep 11 '16 at 20:32

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.