Roman Numeral Converter

Arlen · September 20, 2024, 9:21pm

I just finished my converter. It doesn’t really feel finished because of how it looks visually. I would appreciate any suggestions on how I could make it look better and feedback on my javascript code.

Github
Website

const inputBox = document.getElementById("number");
const converBtn = document.getElementById("convert-btn");
const outputBox = document.getElementById("output");
// rn stands for romand numerals and an stands for Arabic numerals
const romanArr = [
  { rn: "M", an: 1000 },
  { rn: "CM", an: 900 },
  { rn: "D", an: 500 },
  { rn: "CD", an: 400 },
  { rn: "C", an: 100 },
  { rn: "XC", an: 90 },
  { rn: "L", an: 50 },
  { rn: "XL", an: 40 },
  { rn: "X", an: 10 },
  { rn: "IX", an: 9 },
  { rn: "V", an: 5 },
  { rn: "IV", an: 4 },
  { rn: "I", an: 1 },
];

function convertToRoman(input) {
  let resultNumerals = "";
  for (const x of romanArr) {
    while (input >= x.an) {
      resultNumerals += x.rn;
      input -= x.an;
    }
  }
  return resultNumerals;
}
function showOutput() {
  if (inputBox.value === "") {
    outputBox.innerText = "Please enter a valid number";
  } else if (inputBox.value < 1) {
    outputBox.innerText = "Please enter a number greater than or equal to 1";
  } else if (inputBox.value >= 4000) {
    outputBox.innerText = "Please enter a number less than or equal to 3999";
  } else {
    let numberal = convertToRoman(inputBox.value);
    outputBox.innerText = numberal;
  }
}

inputBox.addEventListener("keydown", (e) => {
  if (e.key === "Enter") {
    showOutput();
    inputBox.value = "";
  }
});
converBtn.addEventListener("click", () => {
  showOutput();
  inputBox.value = "";
});

Teller · September 21, 2024, 1:03am

Hi @Arlen

Here are some suggestions for the css:

give the header, div and p elements more padding so the text has some room
decrease the border-radius
I noticed you defined some variable colours, however they are not all used. the #output background-color
how about a slab / tablet effect for the output, something similar to how it is carved on a building.
consider using an older style Roman font, early to mid 1400s

I found the code easier to read with the comment.

Happy coding

sanity · September 21, 2024, 3:03pm

Some improvements could use various naming across the code. For example:

romanArr, rn and an properties of the objects. Especially since comment is there to explain the properties.
showOutput function, which seems to not only show the output, but first figure out what it should be.

Additionally in the for (const x of romanArr) {, instead of using x the object could be destructured. Which would make it a bit cleaner.

Similarities between both callback functions seems a bit repetitive. They are not long, but being close by is slightly highlighting that for most part they are the same.

alkapwn3d · September 21, 2024, 4:16pm

your solution is pretty good. maybe to knit pick a little, maybe move your input validation to its own function outside of showOutput(). Ideally a function should only do one job. so maybe one function to get the error message, and then another just for showing the output.

also i personally find too many else statements confusing. just use if and return whatever didnt get caught.

converBtn looks like a typo.

lasjorg · September 21, 2024, 5:30pm

If you use the "change" event, you do not need to check the key.

As said, you should name your variables better. Definitely do not use code comments to explain variable names.

Arlen · September 21, 2024, 6:39pm

I tried to follow your suggestions to the best of my ability. It still looks bad but not as bad as before. If you have any more suggestions, I would be happy to make the changes again. Thanks for taking the time review my project.

Arlen · September 22, 2024, 12:10am

thank you reviewing my code twice in a row. I think you also reviewed by first javascript project. I’ve destrucured as suggested and change my variables so that they require no comments to explain.

Arlen · September 22, 2024, 12:19am

const inputBox = document.getElementById("number");
const convertBtn = document.getElementById("convert-btn");
const outputBox = document.getElementById("output");
const romanArr = [
  { roman: "M", arabic: 1000 },
  { roman: "CM", arabic: 900 },
  { roman: "D", arabic: 500 },
  { roman: "CD", arabic: 400 },
  { roman: "C", arabic: 100 },
  { roman: "XC", arabic: 90 },
  { roman: "L", arabic: 50 },
  { roman: "XL", arabic: 40 },
  { roman: "X", arabic: 10 },
  { roman: "IX", arabic: 9 },
  { roman: "V", arabic: 5 },
  { roman: "IV", arabic: 4 },
  { roman: "I", arabic: 1 },
];

function convertToRoman(input) {
  let resultNumerals = "";
  for (const { roman, arabic } of romanArr) {
    while (input >= arabic) {
      resultNumerals += roman;
      input -= arabic;
    }
  }
  return resultNumerals;
}

function showError(value) {
  if (value === "") {
    return "Please enter a valid number";
  } else if (value < 1) {
    return "Please enter a number greater than or equal to 1";
  } else if (value >= 4000) {
    return "Please enter a number less than or equal to 3999";
  }
}


function showOutput() {
  outputBox.innerText = showError(inputBox.value) ? showError(inputBox.value) : convertToRoman(inputBox.value)
 
}

inputBox.addEventListener("keydown", (e) => {
  if (e.key === "Enter") {
    showOutput();
    inputBox.value = "";
  }
});
convertBtn.addEventListener("click", () => {
  showOutput();
  inputBox.value = "";
});

This is good advice. I separated the show error and show output as suggested.

I tried to clean it up a bit . The showError() returns instead of assigning to variable. I’m not sure how to reduce the amount of if else statements further.

I fixed the misspelling. Thanks for looking at my code.

Arlen · September 22, 2024, 12:25am

I’ve changed the variable names.

Can you elaborate on this? When I’ve googled how to do this everyone thread I saw used ‘keydown’. I’m not sure how to do it the with ‘change’ event.

lasjorg · September 22, 2024, 4:12pm

The change event will fire when the user presses Enter, so you do not need to check the key.

Just replace keydown with change and try it.

Coder01 · September 23, 2024, 7:10am

I have suggestion on this JS code where we can somehow optimize.

1. Use parseInt to Ensure Input is Treated as a Number

let input = parseInt(inputBox.value);

You are using inputBox.value, which returns a string. To ensure proper numerical comparisons and avoid unexpected behavior, convert the input to an integer using parseInt.

2. Avoid Clearing the Input on Every Event

Clearing the input immediately after a keydown event might be inconvenient for users. You could keep the input intact for multiple conversions unless explicitly cleared.

3. Early Return for Validation

Instead of nesting multiple if statements, use early return for cleaner and more readable code. This reduces nesting and enhances maintainability.

4. Use reduce to Optimize the Conversion Process

function convertToRoman(input) {

return romanArr.reduce((acc, x) => {*
```
 while (input >= x.an) {*
```
```
   acc += x.rn;*
```
```
   input -= x.an;*
```
```
 }*
```
```
 return acc;*
```
}, “”);*
}*

Instead of using a for...of loop, you could use a more functional programming style using reduce, which could make the code shorter and more efficient.

5. Add event.preventDefault() in Keydown

inputBox.addEventListener(“keydown”, (e) => {

if (e.key === “Enter”) {*

 e.preventDefault();  // Prevents any unwanted form submission*

```
 showOutput();*
```
```
 inputBox.value = "";*
```
}*
});*

This ensures that the default form behavior (if any) doesn’t interfere when pressing “Enter”.

This points will make your JS code more effective & fast. In this, there is only one media file or screenshot cause when I tried to upload multiple screenshots of updated code it gave me error. So, I uploaded only one but instead I wrote text form code.
If you have any questions or suggestion feel free to ask.

Arlen · September 23, 2024, 6:05pm

Values are now using parseInt()
Convert buttons no longer clear the input. Instead a reset button shows up that will clear input and output when clicked.
Converter now uses reduce instead of for loop.
prevent default for input may be unnecessary because they are not within a form element.

const inputBox = document.getElementById("number");
const convertBtn = document.getElementById("convert-btn");
const outputBox = document.getElementById("output");
const romanArr = [
  { roman: "M", arabic: 1000 },
  { roman: "CM", arabic: 900 },
  { roman: "D", arabic: 500 },
  { roman: "CD", arabic: 400 },
  { roman: "C", arabic: 100 },
  { roman: "XC", arabic: 90 },
  { roman: "L", arabic: 50 },
  { roman: "XL", arabic: 40 },
  { roman: "X", arabic: 10 },
  { roman: "IX", arabic: 9 },
  { roman: "V", arabic: 5 },
  { roman: "IV", arabic: 4 },
  { roman: "I", arabic: 1 },
];

function convertToRoman(input) {
  return romanArr.reduce((result, { roman, arabic }) => {
    while (input >= arabic) {
      result += roman;
      input -= arabic;
    }
    return result;
  }, "");
}

function showError(value) {
  if (value === "") {
    return "Please enter a valid number";
  } else if (value < 1) {
    return "Please enter a number greater than or equal to 1";
  } else if (value >= 4000) {
    return "Please enter a number less than or equal to 3999";
  }
}

function showOutput() {
  let parsed = parseInt(inputBox.value);
  outputBox.innerText = showError(inputBox.value)
    ? showError(inputBox.value)
    : convertToRoman(parsed);
}

function reset() {
  if (document.getElementById("reset") === null) {
    const clear = document.createElement("button");
    clear.id = "reset";
    clear.innerText = "CLEAR";
    document.getElementById("output-container").append(clear);
    document.getElementById("reset").addEventListener("click", () => {
      outputBox.innerText = "";
      inputBox.value = "";
      clear.remove();
    });
  }
}

inputBox.addEventListener("change", (e) => {
  showOutput();
  reset();
});
convertBtn.addEventListener("click", () => {
  showOutput();
  reset();
});

Thank you for reviewing my code.

lasjorg · September 23, 2024, 7:41pm

I would use CSS to show and hide the reset button. It is more performant and less code than adding and removing the button over and over again.

Arlen · September 23, 2024, 10:37pm

*Created the button on HTML
*Hid the button with CSS
*Used buttons to add and remove display

function showReset() {
  resetBtn.classList.remove("hidden");
}

resetBtn.addEventListener("click", () => {
  inputBox.value = "";
  outputBox.innerText = "";
  resetBtn.className = "hidden"
})

inputBox.addEventListener("change", (e) => {
  showOutput();
  showReset();
});
convertBtn.addEventListener("click", () => {
  showOutput();
  showReset();
});

Thanks again.