JavaScript Algorithms and Data Structures Projects: Roman Numeral Converter Feedback

Code Feedback
1

Hello I have just completed the Roman Numerals Converter JS project and I would really appreciate your feedback on my project.

My code is working correctly for all cases, but after looking at solutions of others I saw much more elegant and efficient-looking solutions than mine, such using recursions or more built-in functions of JavaScript.

For me my code makes perfect sense, while I’m struggling to understand the logic behind some of the posted solutions, but should I be looking into refactoring my code before adding it to my portfolio?

Thank you for your feedback,
Best regards!

function convertToRoman(num) {
  // only works up to 9999
  let numString = num.toString();
  let numSplit = {'thousands' : 0,
                  'hundreds': 0,
                  'tens' : 0,
                  'ones' : 0};
  if (numString.length == 1) {
    numSplit['ones'] = num;
  } else if (numString.length == 2) {
    numSplit['ones'] = Number.parseInt(numString[numString.length-1]);
    numSplit['tens'] = Number.parseInt(numString[0] + '0');
  } else if (numString.length == 3) {
    numSplit['ones'] = Number.parseInt(numString[numString.length-1]);
    numSplit['tens'] = Number.parseInt(numString[1] + '0');
    numSplit['hundreds'] = Number.parseInt(numString[0] + '00');
  } else {
    numSplit['ones'] = Number.parseInt(numString[numString.length-1]);
    numSplit['tens'] = Number.parseInt(numString[2] + '0');
    numSplit['hundreds'] = Number.parseInt(numString[1] + '00');
    numSplit['thousands'] = Number.parseInt(numString[0] + '000');
  }
  
  let romanSymbols = ['I', 'V', 'X', 'L', 'C', 'D', 'M'];
  let romanNumOnes = '';
  let romanNumTens = "";
  let romanNumHundreds = "";
  let romanNumThousands = "";
  
  for (let prop in numSplit) {
    if (prop == 'ones') {
      let currVal = numSplit[prop];
	  
      if (currVal < 4) {
        for (let i=0; i < currVal; i++) {
          romanNumOnes += romanSymbols[0];
        } 
      } else if (currVal == 4) {
          romanNumOnes = romanSymbols[0] + romanSymbols[1];
      } else if (currVal > 4 && currVal < 9) {
          romanNumOnes = romanSymbols[1];
          for (let i=5; i < currVal; i++) {
            romanNumOnes += romanSymbols[0];
          }
      } else {
          romanNumOnes = romanSymbols[0] + romanSymbols[2];
      }  
    } else if (prop == 'tens') {
      let currVal = Number.parseInt(numSplit[prop]
                                   .toString()
                                   .slice(0,1));
      if (currVal < 4) {
        for (let i=0; i < currVal; i++) {
          romanNumTens += romanSymbols[2];
        } 
      } else if (currVal == 4) {
          romanNumTens = romanSymbols[2] + romanSymbols[3];
      } else if (currVal > 4 && currVal < 9) {
          romanNumTens = romanSymbols[3];
          for (let i=5; i < currVal; i++) {
            romanNumTens += romanSymbols[2];
          }
      } else {
          romanNumTens = romanSymbols[2] + romanSymbols[4];
      }  
    } else if (prop == 'hundreds') {
      let currVal = Number.parseInt(numSplit[prop]
                                   .toString()
                                   .slice(0,1));
      if (currVal < 4) {
        for (let i=0; i < currVal; i++) {
          romanNumHundreds += romanSymbols[4];
        } 
      } else if (currVal == 4) {
          romanNumHundreds = romanSymbols[4] + romanSymbols[5];
      } else if (currVal > 4 && currVal < 9) {
          romanNumHundreds = romanSymbols[5];
          for (let i=5; i < currVal; i++) {
            romanNumHundreds += romanSymbols[4];
          }
      } else {
          romanNumHundreds = romanSymbols[4] + romanSymbols[6];
      }  
    } else if (prop == 'thousands') {
      let currVal = Number.parseInt(numSplit[prop]
                                   .toString()
                                   .slice(0,1));
      for (let i=0; i < currVal; i++) {
        romanNumThousands += romanSymbols[6];
      }
    }
  }
  let finalRomanNumeral = romanNumThousands + romanNumHundreds + romanNumTens + romanNumOnes;
  return finalRomanNumeral
}
2

There is quite some room for improvement, but it’s totally fine - everything comes with experience. My suggestion, before going purely imperative with all if ... else statements, try to remember if there are any pre-built native JS solutions or methods that would solve the problem. For example, consider this:

const [one = 0, ten = 0, hrd = 0, tsd = 0] = [...num.toString()]
  .reverse()
  .map((n, i) => +n * 10 ** i);
const numSplit = { one, ten, hrd, tsd };

…a lot of useful functionality is already pre-built making JS (one of) the most powerful functional programming language

1 Like
3

My second advice - look for patterns. In problem solving (and math in general) “pattern” is a synonym of “solution”. For example, convert this to Roman Numbers:

_  _  1  2  3    // I
4  5  6  7  8    // V
9 10             // X
...
 _   _ 10 20 30    // X
40  50 60 70 80    // L
90 100             // C

Look similar, aren’t they?

1 Like
4

Thank you, I will look into my code again and refactor it!

Спасибо! :slight_smile:

5

I have refactored my code and was able to reduce lines of code to twice less the original size.

function convertToRoman(num) {
  // only works up to 9999
  const [ones = 0, tens = 0, hundreds = 0, thousands = 0] = [...num.toString()]
        .reverse()
        .map((n, i) => +n * 1 ** i);
  const numSplit = { ones, tens, hundreds, thousands };
  
  const romanSymbols = [['I', 'V', 'X'], ['X', 'L', 'C'], ['C', 'D', 'M'], ['M']];
  let romanNumsSeparated = ["", "", "", ""];
  
  function indNumConverter (groupNum, currVal) {
	if (currVal < 4) {
		for (let i=0; i < currVal; i++) {
			romanNumsSeparated[groupNum] += romanSymbols[groupNum][0];
        } 
    } else if (currVal == 4) {
        romanNumsSeparated[groupNum] = romanSymbols[groupNum][0] + romanSymbols[groupNum][1];
    } else if (currVal > 4 && currVal < 9) {
        romanNumsSeparated[groupNum] = romanSymbols[groupNum][1];
        for (let i=5; i < currVal; i++) {
			romanNumsSeparated[groupNum] += romanSymbols[groupNum][0];
        }
    } else {
          romanNumsSeparated[groupNum] = romanSymbols[groupNum][0] + romanSymbols[groupNum][2];
    } 
  }
	
  for (let prop in numSplit) {
    if (prop == 'ones') {
	  indNumConverter(0, numSplit[prop]); 
	  
    } else if (prop == 'tens') {
		indNumConverter(1, numSplit[prop]); 
		
    } else if (prop == 'hundreds') {
		indNumConverter(2, numSplit[prop]);
		
    } else if (prop == 'thousands') {
      for (let i=0; i < numSplit[prop]; i++) {
        romanNumsSeparated[3] += romanSymbols[3][0];
      }
    }
  }
  return romanNumsSeparated
						.reverse()
						.join('');
}

convertToRoman(36);
6

Could you please explain what does the “map” method is doing in this case specifically? I don’t quite understand what “+n * 10 ** i” is doing in this code.

Thank you in advance!

7

+n converts n value to a number, it’s basically the same as doing Number(n)
10 ** i is the same as Math.pow(10, i), so if you have number 324, you’ll first make it a string, then array, then reverse it, so you’ll have ['4', '2', '3']. Then if you multiply each digit to 10 ** index, you’ll have:

[
  4 * 10 ** 0, // same as 4 * 1 => 4
  2 * 10 ** 1, // same as 2 * 10 => 20
  3 * 10 ** 2, // same as 3 * 100 => 300
]
1 Like