Can someone give me feedback on my project Code Roman Numeral Converter

[ankurchaulagain]

I solved the challenge but I realize how ineffective it is. Can you give me feedback on my code like and maybe share how did you solved yours and how could I make it better/shorter?
Heard that this was an interview question in some companies. Will probably fail the interview with a long convoluted code like this. A lot of actions are being repeated and I think it could be shorted with some short of recursion but not exactly sure.

  **My code so far**

Click here to view my solution of Roman Numeral Converter

function convertToRoman(num) {
let valR = { 1: 'I', 5: 'V', 10: 'X', 50: 'L', 100: 'C', 500: 'D', 1000: 'M' };
let len = num.toString().length;
if (len === 1 && valR.hasOwnProperty(num)) {
  return valR[num];
}
let firstD, remD, roman = [];
rome(num);
function rome(num) {
  len = num.toString().length;
  switch (len) {
    case 1:

      if (valR.hasOwnProperty(num)) { roman.push(valR[num]); }
      else if (num <= 3) {
        roman.push(valR[1].repeat(num));
      }
      else if (num - 5 >= 4) {
        roman.push(valR[1]);
        roman.push(valR[10]);
      }
      else if (num - 5 < 4) {
        if (num - 5 > 0) {
          roman.push(valR[5]);
          roman.push(valR[1].repeat(Math.abs(num - 5)))
        }
        else {
          roman.push(valR[1].repeat(Math.abs(num - 5)));
          roman.push(valR[5]);
        }
      }

      break;

    case 2:
      firstD = parseInt(num / 10);  //9
      remD = num % 10;  //7
      if (firstD <= 3) {
        roman.push(valR[10].repeat(firstD));
        rome(remD);
      }
      else if (firstD - 5 >= 4) {
        roman.push(valR[10].repeat(10 - firstD));
        roman.push(valR[100]);
        rome(remD);
      }
      else if (firstD - 5 < 4) {
        if (firstD - 5 > 0) {
          roman.push(valR[50]);
          roman.push(valR[10].repeat(Math.abs(firstD - 5)))
        }
        else {
          roman.push(valR[10].repeat(Math.abs(firstD - 5)));
          roman.push(valR[50]);
        }
        rome(remD);
      }


      break;

    case 3:
      firstD = parseInt(num / 100);//6
      remD = num % 100; //49
      if (firstD <= 3) {
        roman.push(valR[100].repeat(firstD));
        rome(remD);
      }
      else if (firstD - 5 >= 4) {
        roman.push(valR[100].repeat(10 - firstD));
        roman.push(valR[1000]);
        rome(remD);
      }
      else if (firstD - 5 < 4) {
        if (firstD - 5 > 0) {
          roman.push(valR[500]);
          roman.push(valR[100].repeat(Math.abs(firstD - 5)))
        }
        else {
          roman.push(valR[100].repeat(Math.abs(firstD - 5)));
          roman.push(valR[500]);
        }
        rome(remD);
      }

      break;

    case 4:
      firstD = parseInt(num / 1000);
      if (firstD <= 3) {
        roman.push(valR[1000].repeat(firstD));
      }
      remD = num % 1000;
      rome(remD);
      break;
  }
}

roman = roman.join("");
console.log(roman);



return roman;
}
convertToRoman(3999);

  **Your browser information:**

User Agent is: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36 OPR/76.0.4017.177

Challenge: Roman Numeral Converter

Link to the challenge:

freeCodeCamp.org

[DanCouper]

So first pass:

let valR = { 1: 'I', 5: 'V', 10: 'X', 50: 'L', 100: 'C', 500: 'D', 1000: 'M' };

I would move this outside the function. It is completely static, never changes, it’s just something used to lookup values. Your choice though, moving it just means that the lookup object isn’t recreated every time you run the function.

let len = num.toString().length;
if (len === 1 && valR.hasOwnProperty(num)) {
  return valR[num];
}

You’re repeating this code further down in the switch: if I delete this, the code still works fine.

let firstD, remD, roman = [];

This is ok, I’ll do a second pass to show why you probably don’t need to do it this way but for now, it’s ok.

One thing I would say is that (and this is my personal opinion, just from experience) is that this is not very clear: it makes it slightly difficult to scan read and edit. The comma operator (this is the thing you are using here) is not used very much in JS – its main use-case IRL is for the purpose here, where you initialise multiple variables on a single line. I would say (again, this is a preference, but in general I’d say it was convention) prefer

let firstD;
let remD;
let roman = [];

Anyway. So now you have this recursive function.

rome(num);
function rome(num) {

It’s slightly confusing calling the function before it’s defined in the code – it works, JS hoists functions to the top of the scope, but it’s not very clear. Ideally, define the function then use it.

And if you are going to use a recursive function here, then build the output string up: what you’re doing is using the recursive function to adjust the values of the three variables you defined earlier. So you have multiple states to manage. This gets a bit confusing. So what you could do is remove two of those variables, just leaving the roman array.

This is simple: switch has no scoping. if blocks do, so use them instead and delete the breaks:

if (len === 1) {
  // same
} else if (len === 2) {
  let firstD = parseInt(num / 10);
  let remD = num % 10;
  // same
} else if (len === 3) {
  let firstD = parseInt(num / 100);
  let remD = num % 100;
  // same
} else if (len === 4) {
  let firstD = parseInt(num / 1000);
  if (firstD <= 3) {
    roman.push(valR[1000].repeat(firstD));
  }
  let remD = num % 1000;
  // same
}

You push into an array, then join the array, why not just use a string in the first place? So replace

let roman = [];

With

let roman = "";

Then replace all

roman.push(valR[n].maybeDoSomethingElse());

With

roman += valR[n].maybeDoSomethingElse();

Then delete roman = roman.join("")

---

Now, one of the things you’re doing here is checking the length of a string version of num, then you’re doing maths on the num to get the magnitude, then working on that and so on.

So why not remove the length check and just work on the number?

Delete

let len = num.toString().length;

Then reverse the if statements: you want the largest first.

Then you can do this:

if (num / 1000 > 1) {
  // we've got 4 digits, do your stuff
} else if (num / 100 > 1) {
  // we've got 3 digits, do your stuff
} else if (num / 10 > 1) {
  // we've got 2 digits, do your stuff
} else {
  // it's 1 digit.
}

You have a recursive solution, so just make the whole function recursive. Move all the code in rome to the main function, move roman = "" into the function head, delete rome and the call to it at the end:

function convertToRoman(num, roman = "") {
  if (num / 1000 > 1) {
    // we've got 4 digits, do your stuff
    return convertToRoman(remD, roman);
  } else if (num / 100 > 1) {
    // we've got 3 digits, do your stuff
    return convertToRoman(remD, roman);
  } else if (num / 10 > 1) {
    // we've got 2 digits, do your stuff
    return convertToRoman(remD, roman);
  } else {
    // it's 1 digit.
    return roman;
  }
}

Cooking on gas now. There’s still a lot of repetition here, but it should be fairly clear what’s happening here: if we have a four digit number < 4000, run the first block, then pass modulo 1000 to the function again, run the second block, then pass modulo 100 to the function again, run the third block then pass modulo 10 to the function again, run the final block and return the result.

So this should all still work exactly the same, if you’re following me. The main issue now is the fiddly number checking in conditionals. Lots of it is almost exactly the same, and it’s all quite hard to follow. Do you need to do any of it? No, is the short answer.

Adjust the lookup object to include the combined numerals (IV, XL etc). This removes the need to figure out if you’re on 4, or 400 or whatever:

const valR = {
  1: 'I',
  4: 'IV',
  5: 'V',
  9: 'IX',
  10: 'X',
  40: 'XL',
  50: 'L',
  90: 'XC',
  100: 'C',
  400: 'CD',
  500: 'D',
  900: 'CM',
  1000: 'M'
};

The function should now break if you try to run it (or at least it’ll give you weird output).

You do checking based on division and modulo. Do you need that? Nope. You are building up a set of numerals. You have the numbers they map to, and you have the input number. So say I list the numbers to map to:

1000, 900, 500, 400, 100, 90, 50, 40, 10, 5, 4, 1

So lets say your input number is 3999. What’s the first number I can subtract from that to leave a value > 0? It’s 1000, leaving 2999. 1000 is “M”. First number again? 1000, leaving 1999. 1000 is “M”. “MM”. First number? 1000, leaving 999. “MMM”. First number? 900, leaving 99. “MMMCM”. First number? 90, leaving 9. “MMMCMXC”. First number? 9, leaving 0. “MMMCMXCIX”.

So lets adjust the function again:

function convertToRoman(num, roman = "") {
  if (num === 0) {
    return roman;
  } else {
    // you decrement your number and build your string here
    return convertToRoman(newNumber, currentRomanNumeralString)
}

I’ll leave filling it in to you (hint: should be just a few lines, and you’ll need a loop)

Note that there’s no need for this to be recursive – it might be clearer if it isn’t, but completely down to you. There are other ways to do this as well (object keys are strings! So be careful, maybe you want invert the object (so it’s like I: 1, IV: 4), or use a Map, or use an array, or whatever)

[lincore]

The algorithm works the same, no matter if the number is 3, 30, 300 or 3000. Only the digits/symbols change (for one, five and ten), not the actual behaviour. So it makes sense to remove them from the algorithm. If you have an array of these digit triplets and the corresponding value of the first digit, you can use map to feed them into the algorithm.

Ex 1: Roman digits as data

const romanSystem = [
    {position: 1000, digits: ['M', '?', '?']},
    {position:  100, digits: ['C', 'D', 'M']},
    {position:   10, digits: ['X', 'L', 'C']},
    {position:    1, digits: ['I', 'V', 'X']},
];

As for the algorithm itself, I think it should handle these four cases:

• I to III
• IV and V
• VI - VIII
• IX and X

This could be reduced to two cases:

• V?I to V?III: nothing or V + 1 to 3 I's
• I(V|X) and (V|X): V or X and maybe an I

That’s quite abstract, though and probably not very readable, so I prefer the first approach.

Ex 2: Once the digits have been abstracted away, the algorithm works for all orders of magnitude (although the provided digits only support num < 4000)

function convertToRoman(num) {
    let remainder = num;
    // f contains the actual algorithm:
    const f = ({position, digits: [one, five, ten]}) => {
        const x = Math.floor(remainder / position);
        remainder %= position;
        return ( x <  4?  one.repeat(x) // I - III
               : x <= 5?  `${one.repeat(5-x)}${five}` // IV - V
               : x <  9?  `${five}${one.repeat(x-5)}` // VI - VIII
               : `${one.repeat(10 - x)}${ten}` // IX - X
               );
    }
    return romanSystem.map(f).join('');
}

[JeremyLT]

Honestly, there is only one case to handle. You can build the output string with a single loop over a set of key-value pairs representing the Roman numerals with an inner loop decrementing your num and adding to your output string while each Roman numeral can be created from the remaining portion of the num.

Now, seeing this one case and reducing/simplifying your code to get to only one case takes some insight and practice. The first step in getting there is looking for everywhere you repeat code or logic with minimal changes. Those portions of your code should be targets for refactoring.

[twotani]

How can we solve this problem? We start from looking for common patterns.

Approach 1:
First thing we need to realize is that the pattern for converting the unit, ten’s, and hundred’s position are the same except for the Roman numerals used. If we spell this out in a big if statement it would be like

if (v == 1) {
   // 'I'
else if (v == 2)  {
  // ' II'
. . .
else if (v==8) {
   // 'VIII'
} else {
  // 'IX'
}

for all three positions with just the difference in Roman numerals used. Realizing this common pattern, we ask ourselves “can we design a function that takes cares of it?” How can we factor out the commonality and adjust for the difference? What kind of arguments can we pass to the function?

We can pass the digit in the unit, ten’s, or hundred’s position and an array of the matching Roman numerals.

['I', 'II', 'III', 'IV', . .  . , 'IX']
['X', 'XX', 'XXX, 'XL', . .  . , 'XC']
['C', 'CC', 'CCC, 'CD', . .  . , 'LM']

We have something like

function convert(digit, romans) {
   . . . 
   return romanNum
}

We call this function three times. We can make three separate calls or use a simple loop.
Just implementing this way should make the code much simpler and easier to follow than your present code. But we can make an improvement to the convert function by eliminating the long if statement. The digit value and the position of the corresponding Roman numeral is the same if we simply add an empty string at the beginning:

['','I', 'II', 'III', 'IV', . .  . , 'IX']

We return romans[digit] as an answer. No more big if statement. This is a data-driven approach, namely, you replace the control flow (if) with a table (array) lookup.

The thousand’s position is different from the other three, but we can handle the digit in the thousand’s position by calling the convert function also. Or we can process it differently.

Approach 2:
Approach 1 uses a table loop technique to handle digits in three different positions. Instead of trying to convert the digits in the number individually, we can solve this problem by the subtraction method. We start with a number, say, 567. How do we know which Roman numerals to use for this number? We need ‘D’ because it’s more than 500. What are other Roman numerals? We already took care of 500, so we need to find out which Roman numerals we need for 67. We need ‘LX’ for 60. How do we know that? For 67, we subtract 50 from 67 and the remainder is 17. We take care of 50 part of 67 by adding ‘L’ to the answer. We repeat the process until the value becomes zero. As another example, if the number is 30, we keep subtracting 10 , and will reach 0 after subtracting 3 times. Thus the answer is ‘XXX’.
So here, the logic is quite straightforward, you keep on subtracting a value from the number until the number becomes zero. For each subtraction, add the corresponding Roman numeral to the answer. This is what Dungeon Master is referring as “one case” in which there’s a number and we keep on subtracting. To make this approach work, we need to figure out which values are we going to subtract and in what order. I gave examples of subtracting 500, 50, and 10. What others do we need?

[ankurchaulagain]

Thank you all for the detailed explanation guys. I appreciate it a lot. Going to traverse through/study all of your solutions to prepare well for the last challenge where one has to create an algorithm to divide the change and implement it.

[ankurchaulagain]

For 567, we would still need to subtract 5, and two 1’s. Like the first answer said make a table of 1 4 5 9 and 10 places that cannot be reached using others right?

[ankurchaulagain]

Can you help me understand your algorithm a little more like with a number say 567?

After that remainder =567 right?

After that we don’t have any parameters to supply to the function f? Or do we make a separate calculation to figure out the position and look it up using the previously supplied objects?

Say somehow we get the position as 100.

So now x=5 and remainder=67 right?

I did not get the next line too like where have we defined one? And things like `$ and what it does.

Will you please explain or help me go through it using a number or two say 567 or 1984?

[twotani]

Yes. Here’s the flow:

   5 6 7
   5 0 0   ------> D
 - - - - -
     6 7
     5 0   ------> D L
 - - - - -
     1 7
     1 0   ------> D L X
 - - - - -
       7 
       5   ------> D L X V
 - - - - -
       2
       1   ------> D L X V I
 - - - - -
       1
       1   ------> D L X V I I
 - - - - -
       0

Notice how we build the answer for Roman numerals from left to right, appending smaller and smaller units. For this example, we “started” from 500, but for a number like 345, we would “start” from 100, because 100 (C) is the largest Roman unit you can subtract without resulting in negative value.

[twotani]

This suggested approach is basically what I called Approach 1. In my explanation, I used specific Roman numerals for all digits like

['', 'I', 'II', 'III', 'IV', . .  . , 'IX']
['', 'X', 'XX', 'XXX, 'XL', . .  . , 'XC']
['', 'C', 'CC', 'CCC, 'CD', . .  . , 'LM']

Instead of that, we can just use

['I', 'V', 'X']
['X', 'L', 'C']
['C', 'D' , 'M']

The code uses map to access each digit. For each digit, you use the corresponding digits like ['X', 'L', 'C']
For example, the expression

x <= 5?  `${one.repeat(5-x)}${five}`

will produce XL if x is 4 and L if x is 5 (for the ten’s position digit).

[ankurchaulagain]

I understood your other approach very well but still don’t understand this one.

I mean what is one? I don’t see any variable defined as one here? Can you please traverse through the conversion like you did with 567 using the first method. The $ is used to add variables to string right ${ } I get this too now. But what is one? What is five? Oh I think I see those are being suppied to the fucntion f right?

Now next confusion is in the code given above where do we supply things to the f function? It’s missing right that is we have to define it somewhere.

const romanSystem = [
    {position: 1000, digits: ['M', '?', '?']},
    {position:  100, digits: ['C', 'D', 'M']},
    {position:   10, digits: ['X', 'L', 'C']},
    {position:    1, digits: ['I', 'V', 'X']},
];

function convertToRoman(num) {
    let remainder = num;
    // f contains the actual algorithm:
//------------>Where do we supply values to the f function?? <----------------------
    const f = ({position, digits: [one, five, ten]}) => {
        const x = Math.floor(remainder / position);
        remainder %= position;
        return ( x <  4?  one.repeat(x) // I - III
               : x <= 5?  `${one.repeat(5-x)}${five}` // IV - V
               : x <  9?  `${five}${one.repeat(x-5)}` // VI - VIII
               : `${one.repeat(10 - x)}${ten}` // IX - X
               );
    }
    return romanSystem.map(f).join('');
}

[twotani]

Are you comfortable with the map function?

romanSystem is an array of objects

const romanSystem = [
    {position: 1000, digits: ['M', '?', '?']},
    {position:  100, digits: ['C', 'D', 'M']},
    {position:   10, digits: ['X', 'L', 'C']},
    {position:    1, digits: ['I', 'V', 'X']},
];

The expression

romanSystem.map(f)

will apply the f function to every element of romanSystem. So you will execute these

   f( {position: 1000, digits: ['M', '?', '?']})

   f( {position:  100, digits: ['C', 'D', 'M']})

   f( {position:   10, digits: ['X', 'L', 'C']})

   f( {position:    1, digits: ['I', 'V', 'X']})

The parameter for the function f is {position, digits: [one, five, ten]} so with this call, for example,

   f( {position:   10, digits: ['X', 'L', 'C']})

position is 10, one is ‘X’, five is ‘L’ and ten is ‘C’. This assignment syntax is called destructuring assignment.

[ankurchaulagain]

Thank you. I get the code. Was frustrated still not being able to trace the conversion of number say 3548 through it but took out a pen and paper and went through it and I finally get it.

I think the other algorithm where you simply look up and subtract, is much easier to understand than this, this was a good mental exercise.

Edit: Regarding the map I was just used to arr.map(a=>somefunction) thus didn’t see it immediately.