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

JavaScript
2

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)

3 Likes