Roman Numerals Converter

Code Feedback
1

Hello guys,

I have just created this humble little algorithm for one of the JS projects.

Could you kindly let me know about my probable big chunk of flaws, advice or observations?

Thanks in advance =D

I am here considering this data structure, consisting in an Array of Objects(I preferred it over creating it and wasting more lines of code) which in my opinion is more readable and closer to real-life basic precautions for better readability and reliability.

This recursive function is a reducer, consisting of the following logic:

=> Gets the input number and checks for the closest(biggest) number available.

=> Finds the corresponding number´s numeral (item id) and pushes it into an array.

=> Subtracts the numeral found from the input value and checks for a reminder

=> In case there is a reminder left (input value - numeral found) run the function again

=> When the reminder is 0, join the array and return the function´s value

const alg = (numb, arr) => {
      const romanNumbers = [
        { id: "I", val: 1 },
        { id: "IV", val: 4 },
        { id: "V", val: 5 },
        { id: "IX", val: 9 },
        { id: "X", val: 10 },
        { id: "XL", val: 40 },
        { id: "L", val: 50 },
        { id: "XC", val: 90 },
        { id: "C", val: 100 },
        { id: "CD", val: 400 },
        { id: "D", val: 500 },
        { id: "CM", val: 900 },
        { id: "M", val: 1000 }
      ];
      const reducer = numb =>
        romanNumbers
        .reduce((a, b) => ({ val: b.val > numb ? a.val : b.val }));
      arr.push(...romanNumbers
        .filter(v => v.val === reducer(numb).val)
        .map(v => v.id));
      const res = numb - reducer(numb).val;
      return res > 0 ? alg(res, arr) : arr.join("");
    };
  
  const convertToRoman = num => alg(num, [])
  
  convertToRoman(36);
2

Honestly, I wouldn’t use recursion at all. Loop is almost always better approach.
But I did a quick look and here are couple comments:

const alg = (numb, arr) => {
  const romanNumbers = [
    { id: 'I', val: 1 },
    { id: 'IV', val: 4 },
    { id: 'V', val: 5 },
    { id: 'IX', val: 9 },
    { id: 'X', val: 10 },
    { id: 'XL', val: 40 },
    { id: 'L', val: 50 },
    { id: 'XC', val: 90 },
    { id: 'C', val: 100 },
    { id: 'CD', val: 400 },
    { id: 'D', val: 500 },
    { id: 'CM', val: 900 },
    { id: 'M', val: 1000 },
  ];

  // regular loop would be better, because it allows for early return
  const reducer = (numb) =>
    romanNumbers.reduce((a, b) => ({ val: b.val > numb ? a.val : b.val }));

  // save to variable to avoid unnecessary calls
  const currentVal = reducer(numb).val;

  // filter() + map() could be replaced with find()
  arr.push(romanNumbers.find((v) => v.val === currentVal).id);

  const res = numb - currentVal;

  return res > 0 ? alg(res, arr) : arr.join('');
};

const convertToRoman = (num) => alg(num, []);
3

Thanks very much for the reply! I will keep your advices in mind. Could you please go a little bit deeper regarding the fact that recursion should be avoided in such a way? Is there such a big performance diference in between these two approaches? Considering a same approach, of having an array of numerals with a loop Wouldn´t it lead to an iteration performed a number of times in order to achieve whatever logic is being passed to possibly create an array of numerals and join them in the end anyway?

When I privately save the reducer function into a variable(currentVal in this case), the function saves a snapshot of its pointer(as it is pointing to an object in this case), avoiding the unnecessary calls? Could you please talk about it as well?

I did not not figure about using find() haha. I fragmented the logic in my mind and did not figure this approach =D. Thanks!!

4

I didn’t say that recursion should be avoided. But you should aim to write simple code because code is more often read than it’s written and (at least to me) reading recursive code is harder than reading simple loop.
(Btw, there are languages where recursion is the only way to do looping)

You create function expression, but then you call it two times. To avoid unnecessary second call, save the result of the first call to a variable.

5

Oh, now I see what you´re saying about recursion… Yes the simpler the better…

Well, the deal was that I wanted to use this approach for me to exercise recursion in this implementation. I have been using as many recursion as possible for these last days in order to strengthen this way of thinking.

Once again, thanks very much for sparing some of your time shedding some light into my learning process =D