Roman Numeral Converter Questions

Hey guys, I’m currently working on the JavaScript module’s Roman Numeral Converter challenge, found at https://learn.freecodecamp.org/javascript-algorithms-and-data-structures/javascript-algorithms-and-data-structures-projects/roman-numeral-converterr]. My code is below…

function convertToRoman(num, ret = '') {
    let romNums = {
     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',
     10000: 'Vinculum-X'
 };
    let keys = Object.keys(romNums);
    for(let i = 0; i < keys.length; i++) {
        if(romNums.hasOwnProperty(keys[i])) {
            //keys[i] for keys
            //romNums[keys[i]] for values
            if(num > 0) {
                if(keys[i + 1] > num || keys[i + 1] === null) {
                    ret += romNums[keys[i]];
                    num -= keys[i];
                    return convertToRoman(num, ret);
                }                
            }
        }
    }
    console.log(`ret equals ${ret}`);
    console.log(`num equals ${num}`);
    return ret;
}

console.log(convertToRoman(1000));

I’ve managed to finish the challenge, however there is a noticeable flaw in my code - without the ‘10000’ property in romNums, the tests for 1000 and above fail. I’ve added 10000 with an arbitrary ‘Vinculum-X’ value here, however, it is not truly a Roman Numeral. What can I do to make my code pass the tests without the ‘10000’ property?

Also, if you guys could give me a general review and tips regarding my code, it would be much appreciated;

  • Would it be a good idea in the future to make the function non-mutating by using a ‘temp’ variable in place of ‘num’? Would that be possible with the current code?
  • I’m not sure if the usage of [Object.keys()] and ES6’s [ret = ‘’] are appropriate or standard-practice in cases like these. Especially with the latter - I feel like without relying on this initialisation function, I would struggle with creating an appropriate recursive loop that doesn’t reset ‘ret’ every time. How would I go about this in ES5? Would I need a function inside the function or?
  • Is it a bad idea to use the “if the value of the index above the current index fulfills a certain criteria, do something with the current index” if-condition logic?

Best Regards.

The tests only need to go up to 3999, which is “MMMCMXCIX”, you absolutely shouldn’t need the 10000 property. You should just be adding an M per 1000. There are multiple different [mediæval] additions to Roman numerals to allow larger numbers (beyond 4000), but they’re all quite clumsy because Roman numerals are not good for handling large numbers. Using a vinculum I guess is the most common, but there’s no consistent agreed-upon standard way to handle things (Do you get rid of M entirely? Or do you use M, MM, MM, then IV with vinculum? or I, II, III, IV all with vinculum? :man_shrugging: ). AFAIK, Roman numerals aren’t used for anything that needs numbers that large, so it’s a non-issue.

The function is non-mutating: it takes a number and returns a string, and every recursive call involves a new value. This line technically mutates the two arguments

ret += romNums[keys[i]];
num -= keys[i];

But then thosse two values are passed in as arguments to a new function, and the process begins again. There is mutation, but it happens for a fraction of a fraction of a microsecond. You can just do convertToRoman(num - keys[i], ret + romNums[keys[i]]), which is the same thing, but maybe makes it clearer that these are new values going in.

They’re fine, but you’re overcomplicating things in other ways. The Object.keys seems completely unecessary here: you are iterating over an object every time for no reason. Using a default value to initialise is totally fine for a recursive function and would be the best way to do things.

Putting the function inside another function is also fine, and the reason for doing that here would be to move the reference object and the keys out of the recursive function so that you don’t recreate them every time (but you can still keep the default). That nested function would be effectively replacing a loop statement. In functional languages where recursion is both standard and optimised (for example ML or OCaml), this is a common pattern. I don’t think doing it has much overhead in JS, but I don’t tend to use recursion at all in JS, so from experience I’m not sure.

In this case, probably, because of the way you’ve written the logic. You shouldn’t need to loop: the recursion is the loop.


A possible solution:

  1. Get rid of the 10000 entry
  2. Move the reference object out of the function
  3. Move the Object.keys out of the function.
  4. Replace that Object.keys(numerals) call with an array containing the keys, written in reverse.
  5. Pass this array into the function - so function call is now convertToRoman(num, keys = theNumeralKeys, ret = ''). This is going to remove the need for the index checking.
  6. Move the termination logic to the top of the function: instead of having if(num > 0) { in that loop, have if (num <= 0) return ret at the top of the function.
  7. Get rid of the loop logic.
  8. If num is >= to the first value in keys (for the first iteration, this is 1000), then do a recursive call: return convertToRoman(num - keys[0], keys, ret + keys[0]). The >= is important here, if it is just > the whole thing drops into infinite recursion.
  9. If it isn’t, just need to drop the first value off the array and try again with the same num value: return convertToRoman(num, keys.slice(1), ret)
const romNums = {
  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',
 };

const romNumKeys = [1000, 900, 500, 400, 100, 90, 50, 40, 10, 9, 5, 4, 1];

function convertToRoman(num, [key, ...keys] = romNumKeys, numerals = '') {
  if (num <= 0) return numerals;
  
  if (num >= key) {
    return convertToRoman(num - key, [key, ...keys], `${numerals}${romNums[key]}`);
  } else {
    return convertToRoman(num, keys, numerals);
  }
}
1 Like

Hello!

The problem with your code is that You’re checking for null where You should check for undefined:

if(keys[i + 1] > num || keys[i + 1] === undefined) // This works

Unless You really need to, You should never mutate the parameters.

Think as the user of Your function/program; If You pass a parameter to the function, but do require to further process the value you just passed, the expected value is the same, but if the function mutates it, errors will occur.

That’s fine; it’s just a way of doing it.

Same as before, although as said by @DanCouper, the recursive calls to the function are the loop.

1 Like