Optimizing for() loops on Roman Numeral Converter JS

Question

I'm looking for some feedback on this converter I made. It works just fine for the instructions I had, but I'm wondering what else could be done to improve it or if there's a better aproach to doing this. I wasn't expecting to nail this since I'm just starting to learn JS.

I'm aware I could've implemented things like Math.floor(), Math.min() to make the validations, I'm mostly wondering on the conversion itself, what else could be optimized, I'm sure there was an easier way. I also thought of using forEach() instead of multiple for() and arrays but I'm not sure how

Here's the whole thing in codepen: Roman Numeral Converter

Here's the JS code itself:

const numberInput = document.getElementById("number");
const convertButton = document.getElementById("convert-btn");
const output = document.getElementById("output");



function convertRom () {

  const numbersList = numberInput.value.split((/(\d)/)).filter(Boolean)
  for (let i = numbersList.length; numbersList.length < 4; i--) {
    numbersList.unshift('0');
    i -= 1
  }

  
  console.log("numbersList " + numbersList);
  const romanDigit1 = [];
  const romanDigit2 = [];
  const romanDigit3 = [];
  const romanDigit4 = [];

  for (let digit1 = numbersList[0]; digit1 > 0; digit1 -1){
    romanDigit1.push("M");
    digit1 -= 1
  }

  for (let digit2 = Number(numbersList[1]); digit2 > 0; digit2 === digit2){

      if (digit2 === 9) {
        romanDigit2.push("CM")
        digit2 -= 9;
      } else if (digit2 === 5){
        romanDigit2.unshift("D")
        digit2 -= 5;
      } else if (digit2 === 4){
        romanDigit2.push("CD")
        digit2 -= 4;
      } else {
        romanDigit2.push("C");
        digit2 -= 1 
      }
  } 

  for (let digit3 = Number(numbersList[2]); digit3 > 0; digit3 === digit3){

      if (digit3 === 9) {
        romanDigit3.push("XC")
        digit3 -= 9;
      } else if (digit3 === 5){
        romanDigit3.unshift("L")
        digit3 -= 5;
      } else if (digit3 === 4){
        romanDigit3.push("XL")
        digit3 -= 4;
      } else {
        romanDigit3.push("X");
        digit3 -= 1 
      }
  } 

  for (let digit4 = Number(numbersList[3]); digit4 > 0; digit4 === digit4){
    
      if (digit4 === 9) {
        romanDigit4.push("IX")
        digit4 -= 9;
      } else if (digit4 === 5){
        romanDigit4.unshift("V")
        digit4 -= 5;
      } else if (digit4 === 4){
        romanDigit4.push("IV")
        digit4 -= 4;
      } else {
        romanDigit4.push("I");
        digit4 -= 1 
      }    

  }
  
  const romanResult = romanDigit1.join("") + romanDigit2.join("") + romanDigit3.join("") + romanDigit4.join("");
  output.innerHTML = romanResult;
}



function validate() {
  switch (true){
    case numberInput.value === "":
      output.innerHTML = "Please enter a valid number";
      numberInput.value = "";
      break
    case numberInput.value <= 0:
      output.innerHTML = "Please enter a number greater than or equal to 1";
      numberInput.value = "";
      break
    case numberInput.value >= 4000:
      output.innerHTML = "Please enter a number less than or equal to 3999";
      numberInput.value = "";
      break
      default:
        convertRom();
  }
}


convertButton.addEventListener("click", validate)

Answer 1

A function should have parameters. They should not use global variables as input instead of using parameters.

---

numbersList.length < 4 why is this line here, and more importantly, what is "4" to mean here? This is often referred to as "magic value" and we hope not to have to guess meaning other than at the start of an executable.

---

The numberList generation should be in a specific function that indicates what it actually does.

---

const romanDigit1..4 = [];

Every time that you start to count in your code you're doing something wrong; the computer should do the counting.

By definition a digit can never consist of multiple characters. The Roman system doesn't use "digits" so defining them as an array seems incorrect. What about romanNumerals?

---

for (let digit1 = numbersList[0]; digit1 > 0; digit1 -1){

Code is incorrect: it doesn't work for values over 3999, those should be caught before entering the loop, for instance by adding a guard clause such as:

if (value >= 4000) {
    throw new Error("Function is capped at 3999, which is the maximum classic Roman numerals can encode");
}

Of course the input should try and make sure that the user doesn't enter that value so that the error never gets thrown. Public methods however should always check that the input confirms to the preconditions for successfully executing the method, rather than to return incorrect values.

There is another digit -= 1 later on, which was probably required as digit - 1 doesn't assign a different value to digit1.

---

  for (let digit2 = Number(numbersList[1]); digit2 > 0; digit2 === digit2)

The 3rd term is not required in a for loop. It may be a good idea to always have an expression in there, but there is no need for a filler if it is not needed.

---

The strict equals operator === is not wrong, but as digitN is always of the correct type it is probably easier to just use ==.

NOTE: apparently it is good practice to use === anyway for JavaScript, see the comment below.

---

A slightly smarter algorithm would give a set of roman numerals to a function and then performs the exact same for loop for those. There are even smarter algorithms possible where you just start with a string consisting of all roman numerals. That way it is also easier to extend your implementation to allow higher numbers by simply adjusting the string.

---

default:
    convertRom();

OK, so you ask for a number and if anything other than an empty string is entered it will be converted? That's just weird and should at least be indicated to the user.

---

In general the code is readable & relatively well spaced out. Sometimes it is slightly inconsistent, such as the white line before the for loops that happens most of the time but not always.

---

The identifiers are named OK in general, but here we get into the strange situations where the numbers are actual decimal digits. So, uh, I'd propose decimalsDigits.

Answer 2

Follow the advice of @MaartenBodewes.

Also, this is just weird:

  switch (true) {
    case numberInput.value === "":
      ... break
    case numberInput.value <= 0:
      ... break
    case numberInput.value >= 4000:
      ... break
      default:

JS offers a perfectly nice if ... else if ... else if ... else syntax. Please don't abuse switch in this way. Yes, we have a Turning machine at our disposal, and there are many ways to tell the machine to compute a result. But stick to the conventional approach if you want to effectively communicate your technical ideas to colleagues.

Also, indenting matters to humans -- that default: should be aligned.