Would you say this is incredibly inefficient?

1

Would something like this never, ever be used in actual practice? Is it too “manual-ish”? I’m asking because that’s what I’m leaning towards. Basically, would I get laughed at for this? :smile:

Here’s my solution to the Roman Numeral Converter:

function convertToRoman(num) {
  var numbersArray = (""+num).split("");
  var ones = numbersArray.splice(-1,1);
  var tens = numbersArray.splice(-1,1);
  var hundreds = numbersArray.splice(-1,1);
  var thousands = numbersArray.splice(-1,1);
  function onesRoman() {
    
    var result = "";
    var onesToNumber = ones.map(Number);
    if (onesToNumber <= 3) {
      for(var i = 0; i < onesToNumber; i++) {
        result = result + "I";
      }
    }
    if (onesToNumber == 4) {
      result = "IV";
    }
    if (onesToNumber == 5) {
      result = "V";
    }
    if (onesToNumber > 5 && onesToNumber <=8) {
      result = "V";
      for (var j = 0; j < onesToNumber-5; j++) {
        result = result + "I";
      }
    }
    if (onesToNumber == 9) {
      result = "IX";
    }
    return result;
  }
  
  function tensRoman() {
    var result = "";
    var tensToNumber = tens.map(Number);
    if (tensToNumber <= 3) {
      for(var i = 0; i < tensToNumber; i++) {
        result = result + "X";
      }
    }
    if (tensToNumber == 4) {
      result = "XL";
    }
    if (tensToNumber == 5) {
      result = "L";
    }
    if (tensToNumber > 5 && tensToNumber <=8) {
      result = "L";
      for (var j = 0; j < tensToNumber-5; j++) {
        result = result + "X";
      }
    }
    if (tensToNumber == 9) {
      result = "XC";
    }
    return result;
    
  }
  function hundredsRoman() {
    var result = "";
    var hundredsToNumber = hundreds.map(Number);
    if (hundredsToNumber <= 3) {
      for(var i = 0; i < hundredsToNumber; i++) {
        result = result + "C";
      }
    }
    if (hundredsToNumber == 4) {
      result = "CD";
    }
    if (hundredsToNumber == 5) {
      result = "D";
    }
    if (hundredsToNumber > 5 && hundredsToNumber <=8) {
      result = "D";
      for (var j = 0; j < hundredsToNumber-5; j++) {
        result = result + "C";
      }
    }
    if (hundredsToNumber == 9) {
      result = "CM";
    }
    return result;
    
  }
  function thousandsRoman() {
    var result = "";
    var thousandsToNumber = thousands.map(Number);
    for (var i = 0; i < thousandsToNumber; i++) {
      result = result + "M";
    }
    return result;
  }
  num = thousandsRoman() + hundredsRoman() + tensRoman() + onesRoman();
  return num;
}

convertToRoman(649);

After I finished writing it and had it working, I checked the other answers people had and thought - :no_mouth:

1 Like
2

Maybe you can come up with another way to accomplish the same goal with a few less if statements to make it a little more readable, but as far as efficiency goes, it looks fine. I do not see any unnecessary loops, which would effect the efficiency much more than a few extra if/else statements.

Maybe look at a switch statement to replace the if statements?

2 Likes
3

That’s good to hear that it isn’t that bad. I will try to do what you said. Thanks.