My solution to Roman Numeral Converter, Is it OK?

JavaScript
1

Hello! This is my first comment after starting the curriculum a few months ago and returning to it recently. Before this I completed the CS50, which I think helps me a lot to understand the logic behind these exercises.

I have finished the Roman numeral converter exercise, and although it works I would like to know your opinion on how efficient it can be. I’ve seen other solutions and they are mostly focused differently.

I have removed some parts of the code like the valid number check and the call to the main function .

/* VARIABLES */

const romans = ['I', 'V', 'X', 'L', 'C', 'D', 'M'];
let number = numInput.value;


/* 2 - CONVERT AND PASS VALUE */

function convert(fNum) {
  const fNumStr = fNum.split("");
  const i = fNumStr.length;
  let j = 1;
  let result = "";

  fNumStr.forEach((num) => {
    let char1 = romans[0 + (i - j) * 2];
    let char2 = romans[1 + (i - j) * 2];
    let char3 = romans[2 + (i - j) * 2];
    j++;

    if (num <= 3) {
      numIII(num);
    }
    else if (num == 4) {
      result += char1 + char2;
    }
    else if (num >= 5 && num != 9) {
      result += char2;
      numIII(num - 5);
    }  
    else {
      result += char1 + char3;
    }

    function numIII(value) {
      for (let j = 1; j <= value; j++) {
        result += char1;
      }
    }
  });

  output.innerText = result;
}
2

Hello, it looks good and very consice if this is all of it. Most of the calulations are in micoseconds, so to answer there is no efficient way. Ive seen so many programs written several ways. As a challenge you can take all your code and go the ES6 route using switch statements for if statements or terinary and implicit returns and so on but Ive found that something to do gradually of course. :grinning:

3

Thanks for your answer! I will take into account what you say :slight_smile: could you say that ES6 is like a “good practices” guide to write JS in the best possible way? As I said I’m not very familiar with programming (I’m a designer and video editor) and there are a lot of terms I don’t know.

4

Yes, Once you start using it and become familiar it`s a lot better.

5

I wouldn’t worry as much about using fancy syntax. I’d look at ways to simplify your logic over fancy syntax.

Here I’d use const since then variables don’t change value.

Here, I wouldn’t make the function use values outside of its scope. I’d pass in char1 and return the value to concatenate onto the result.

Also

if ( ) {

} else {

}

is more common with JS than

if ( ) {

} 
else {

}
6

Hey! Thanks for the advice, I’ll keep it in mind!

7

Try to find some better names for the variables, using descriptive names can help greatly with readability.

Additionally, if you want to use forEach, your don’t need to keep j variable and incrementing it on your own. forEach accepts callback with up to three arguments, first one is the current element from array, while the second is the index of the current element.

1 Like
8

Just today, while doing another exercise, I went into the forEach documentation because I didn’t understand a part of it and I saw that the variable j was not necessary at all.

1 Like
9

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.