5
\$\begingroup\$

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)

\$\endgroup\$

2 Answers 2

6
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Ah, I must add that i had instructions for this. Maximum number to be able to convert was 3999. That's why the validate section does what it does. Thank you for your input. How would I go doing "string concatenation"? I'm new to this and some of these concepts are still alien. \$\endgroup\$ Feb 25 at 1:39
  • \$\begingroup\$ Ah, wait, you need it for the unshift capability I guess, which seems OK to me. Pretty sure that it is possible to program it without that (I sure did back in the days) but that would alter the general idea for the method that has been applied. I'll remove that remark. \$\endgroup\$ Feb 25 at 1:48
  • \$\begingroup\$ "The strict equals operator === is not wrong, but as digitN is always of the correct type it is probably easier to just use ==." But if the code changes, then === will catch any type bugs that are introduced. The best practice is to never use ==, which is a code smell at minimum, and use eqeqeq set to always. Otherwise, a good review. \$\endgroup\$
    – ggorlen
    Feb 25 at 3:03
  • \$\begingroup\$ @ggorlen Thanks, I've added this in, pointing to your comment. \$\endgroup\$ Feb 25 at 16:03
2
\$\begingroup\$

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.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I like this use of switch, it captures well the idea of finding the first match. I wouldn't use it however because of the break that must end the block and can easily be forgotten. \$\endgroup\$
    – Florian F
    Feb 25 at 17:32
  • \$\begingroup\$ Thank you for the observation, I originally used if - else if for this but changed it up just to try something different. Again, I'm at a very early learning stage, I'm trying hard to use the tools I've been shown. As for the indentation, I'll be more careful next time :^) \$\endgroup\$ Feb 25 at 22:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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