Roman Numeral Converter: I got it right but Tests returned it all wrong

Tell us what’s happening:
I developed a Roman numeral converter with object lookup and recursive function to convert it. I got it all right when I coded it in VS Code but FCC Tests returned it all wrong. Am I wrong in returning result?

Here is my code:

let convertedNum = [];

function convertToRoman(num) {
    // Object initializer for conversion to Roman numeral ("Roman Table")
    let romanNumProperty = {
        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'
    }
    // Array of keys of the Table, reversed for finding nearest num purpose
    let romanNumKeys = Object.keys(romanNumProperty).reverse();

    // Look for num at the Table, if existed
    if (!romanNumProperty[num]) {
        // Find lower nearest number of romanNumKeys (keys)
        let nearestNum = romanNumKeys.find(x => (x <= num) );

        // If num length is 1 and found a value in object lookup, return it to finish
        // But if num doesn't find a value, process it again untuk it found
        if (num.toString().length === 1 && romanNumProperty[num]) {
            return convertToRoman(num);
        } else {
            while (nearestNum < num) {
                convertedNum.push(romanNumProperty[nearestNum]);
                num -= nearestNum;
                nearestNum = num;
                return convertToRoman(num);
            }
        }

        // Same as above, but for num length > 2
        while (nearestNum < num) {
            convertedNum.push(romanNumProperty[nearestNum]);
            num -= nearestNum;
        }

        return convertToRoman(num);
    }

    // If lookup existed (found a value), simply push it to array
    convertedNum.push(romanNumProperty[num]);

    // ...and join it!
    return convertedNum.join('');
}

convertToRoman(36);
Browser info

User Agent is: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Firefox/102.0

Challenge: Roman Numeral Converter

Link to the challenge:

First, you are getting a reference error because you haven’t properly declared convertedNum. After you do that I would suggest you do

console.log('returning: ', convertedNum.join(''));

right before the last return statement so you can see what is actually being returned.

Also, I don’t think you are implementing recursion properly here. Generally, you want to save off the return value from the recursive call so you can build up the answer. You are just calling the function recursively but not doing anything with the return value from those calls. And some of those return statements are never being executed. You can put console logs before each of them to figure out which ones never get executed.

2 Likes

Hmmm, I declared convertedNum at the top of function as blank array. About the returning, I got same value too.

But when I console-logged every function that I call recursively, I found out that only this coded that gets executed.

When I first developed this, I wanted convertedNum to be inside function, but because I called function recursively, it gets removed and now I’m confused.

So, how can I improve my code? I already removed some parts and only that code above that remains intact.

While you’re away, I decided to rewrite my code with function call removed. Now it works by updating nearestNum after subtracting num:

function convertToRoman(num) {
    let convertedNum = [];
    let romanNumProperty = {
        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'
    }
    let romanNumKeys = Object.keys(romanNumProperty).reverse();

    if (!romanNumProperty[num]) {
        let nearestNum = romanNumKeys.find(x => (x <= num));
        while (nearestNum <= num) {
            convertedNum.push(romanNumProperty[nearestNum]);
            num -= nearestNum;
            nearestNum = romanNumKeys.find(x => (x <= num));
        }
    } else {
        convertedNum.push(romanNumProperty[num]);
    }

    return convertedNum.join('');
}

What do you think? Feedback are appreciated

Great job. I think this is a much better solution than trying to do it recursively. The only suggestion I would make is that you can get rid of the if/else to simplify this a little more. What does the else do? It’s basically just returning romanNumProperty[num] if romanNumProperty[num] is defined. You can do that at the very beginning and then you don’t need an if at all.

I did as you suggested. Like this?

let romanNumKeys = Object.keys(romanNumProperty).reverse();
let nearestNum = romanNumKeys.find(x => (x <= num));

while (nearestNum <= num) {
    convertedNum.push(romanNumProperty[nearestNum]);
    num -= nearestNum;
    nearestNum = romanNumKeys.find(x => (x <= num));
}

return convertedNum.join('');

Did you run this through the tests? Did they all pass?

Happy, happy, joy, joy!

It all passed.