I think I solved the Roman Numeral Converter Project, but FCC disagrees

I believe I have completed the project/challenge for creating a Roman Numeral converter in vanilla JS. However, the FCC platform doesn’t like my solution. I am getting a console log right before my returns that match the test, but as I said, the FCC platform disagrees.

One thing I am questioning is whether or not I am using return correctly in a recursive function. Am I using it correctly?

Any help would be much appreciated!

var myAnswer = “”;
function convertToRoman(num) {
console.log(‘num’, num)
var remaining = num;

var romanNumerals = [‘I’,‘IV’,‘V’,‘IX’,‘X’,‘XL’,‘L’,‘XC’,‘C’,‘CD’,‘D’,‘CM’,‘M’]
var romanValues = [1,4,5,9,10,40,50,90,100,400,500,900,1000]

for(var i = romanValues.length -1; i >= 0; i–){
if (romanValues[i] <= remaining){
console.log(‘roman Value’,romanValues[i]);
myAnswer = myAnswer + romanNumerals[i];
remaining = remaining - romanValues[i];
console.log(‘remaining’,remaining);
if (remaining == 0){
console.log(‘answer:’,myAnswer)
return myAnswer
} else {
return convertToRoman(remaining)
}
}
}
}

convertToRoman(2);

Your code contains global variables that are changed each time the function is run. This means that after each test completes, subsequent tests start with the previous value. To fix this, make sure your function doesn’t change any global variables, and declare/assign variables within the function if they need to be changed.

Example:

var myGlobal = [1];
function returnGlobal(arg) {
  myGlobal.push(arg);
  return myGlobal;
} // unreliable - array gets longer each time the function is run

function returnLocal(arg) {
  var myLocal = [1];
  myLocal.push(arg);
  return myLocal;
} // reliable - always returns an array of length 2
3 Likes

Thanks, you definitely pointed me in the right direction!

However, now that I know what I’m missing, I’m still struggling to find a solution. What I need is the myAnswer variable to go back to an empty string each time I call the convertToRoman function, but then update and add to its self each time it goes through the for-loop.

Here is my current try where I’ve divided up the initial function and the recursive function. Unfortunately its still not working. What I keep getting now is a myAnswer variable that resets to an empty string each time the recursiveRomans function gets called. :frowning:

function convertToRoman(num) {

myAnswer = "";

console.log('num', num);

return recursiveRomans (num, myAnswer);

}

function recursiveRomans (remaining, myAnswer) {

var romanNumerals = ['I','IV','V','IX','X','XL','L','XC','C','CD','D','CM','M']

 var romanValues = [1,4,5,9,10,40,50,90,100,400,500,900,1000]

for(var i = romanValues.length -1; i >= 0; i--){

    if (romanValues[i] <= remaining){

    console.log('roman Value',romanValues[i]);

    var myAnswer = myAnswer + romanNumerals[i];

    console.log('current answer:', myAnswer)

    remaining = remaining - romanValues[i];

   console.log('remaining',remaining);

if (remaining == 0){

    console.log('answer:', myAnswer)

    return myAnswer

       } else {

        return convertToRoman(remaining, myAnswer)

       }

   }

}

}

convertToRoman(16);

I actually just figured out a solution. I passed in myAnswer as a second argument but gave it the default value of myAnswer = “”. The first time around I call convertToRoman with no second argument, and so its populated as emtpy.

var myAnswer = ‘’;

function convertToRoman(num, myAnswer=“”) {

console.log('num', num)

var remaining = num;

var romanNumerals = ['I' , 'IV' , 'V' , 'IX' , 'X' , 'XL' , 'L' , 'XC' , 'C' , 'CD' , 'D' , 'CM' , 'M']

var romanValues = [1,4,5,9,10,40,50,90,100,400,500,900,1000]

for(var i = romanValues.length -1; i >= 0; i--){

    if (romanValues[i] <= remaining){

        console.log('roman Value',romanValues[i]);

        myAnswer = myAnswer + romanNumerals[i];

        remaining = remaining - romanValues[i];

        console.log('remaining',remaining);

        if (remaining == 0){

            console.log('answer:',myAnswer)

            return myAnswer

        } else {

            return convertToRoman(remaining, myAnswer)

        }

    }

}

}

convertToRoman(2);

I suggest trying to solve this without modifying the skeleton code. Also, you just don’t want to be using global variables that way. You don’t have to do that kind of gymnastics, even if you want to make this a recursive function.

Maybe look at Use Recursion to Create a Countdown for ideas on how to do that.

Yeah, you’re right, and the funny thing is I don’t even need that global variable declared now that I have the default argument value.

I still would suggest figuring out how to do this without changing the function signature.