patternjavascriptMinor
Number to Roman in Javascript
Viewed 0 times
javascriptnumberroman
Problem
Is this a good solution for the Number to Roman Numerals Kata in Javascript?
function toRomans(n) {
var nString = n.toString();
var result = "";
var allLetters = [["I", "V", "X"], ["X", "L", "C"], ["C", "D", "M"], ["M"]];
var letterSet = nString.length - 1;
for (var index in nString) {
result = result + conversor(nString[index], allLetters[letterSet]);
letterSet --;
}
console.log(n + " --> " + result);
}
function conversor(digit, letterSet) {
switch(digit) {
case "1":
return letterSet[0];
break;
case "2":
return letterSet[0] + letterSet[0];
break;
case "3":
return letterSet[0] + letterSet[0] + letterSet[0];
break;
case "4":
return letterSet[0] + letterSet[1];
break;
case "5":
return letterSet[1];
break;
case "6":
return letterSet[1] + letterSet[0];
break;
case "7":
return letterSet[1] + letterSet[0] + letterSet[0];
break;
case "8":
return letterSet[1] + letterSet[0] + letterSet[0] + letterSet[0];
break;
case "9":
return letterSet[0] + letterSet[2];
break;
case "0":
// skip character
return "";
break;
default:
break;
}
}
toRomans(2);
toRomans(7);
toRomans(32);
toRomans(456)
toRomans(1243);
toRomans(3500);Solution
Invalid input
There we go. I broke your function. Sorry.
Even though the Roman Numerals Kata specifies that only numbers between 1 and 3000 needs to be returned correctly, I think it is better to handle numbers outside the working range in a specific way, such as throwing an exception.
Return, do not log
Imagine that I am writing a program and I want to use your method to count how many unique characters there is in the resulting string.
This doesn't let me do that, really. It would be much better to do:
And then I can log, show an alert, or count the number of unique characters after calling the function. This is how it is properly done in programming.
Returning and breaking
In your
I would also
I believe the
toRomans(21463);There we go. I broke your function. Sorry.
Even though the Roman Numerals Kata specifies that only numbers between 1 and 3000 needs to be returned correctly, I think it is better to handle numbers outside the working range in a specific way, such as throwing an exception.
Return, do not log
Imagine that I am writing a program and I want to use your method to count how many unique characters there is in the resulting string.
console.log(n + " --> " + result);This doesn't let me do that, really. It would be much better to do:
return result;And then I can log, show an alert, or count the number of unique characters after calling the function. This is how it is properly done in programming.
console.log(toRomans(56)); is more clear what the code is doing than simply toRomans(56);Returning and breaking
In your
switch statement, you don't need both return and break. When calling return, the method ends, so further statements will not be executed. Your breaks are dead code that will never be called.I would also
return ""; in the default case as well, which lets 0 be handled together with default.function conversor(digit, letterSet) {
switch(digit) {
case "1":
return letterSet[0];
case "2":
return letterSet[0] + letterSet[0];
case "3":
return letterSet[0] + letterSet[0] + letterSet[0];
case "4":
return letterSet[0] + letterSet[1];
case "5":
return letterSet[1];
case "6":
return letterSet[1] + letterSet[0];
case "7":
return letterSet[1] + letterSet[0] + letterSet[0];
case "8":
return letterSet[1] + letterSet[0] + letterSet[0] + letterSet[0];
case "9":
return letterSet[0] + letterSet[2];
default:
return "";
}
}I believe the
conversor function can be further optimized, but I'll leave as-is for now.Code Snippets
toRomans(21463);console.log(n + " --> " + result);return result;function conversor(digit, letterSet) {
switch(digit) {
case "1":
return letterSet[0];
case "2":
return letterSet[0] + letterSet[0];
case "3":
return letterSet[0] + letterSet[0] + letterSet[0];
case "4":
return letterSet[0] + letterSet[1];
case "5":
return letterSet[1];
case "6":
return letterSet[1] + letterSet[0];
case "7":
return letterSet[1] + letterSet[0] + letterSet[0];
case "8":
return letterSet[1] + letterSet[0] + letterSet[0] + letterSet[0];
case "9":
return letterSet[0] + letterSet[2];
default:
return "";
}
}Context
StackExchange Code Review Q#83097, answer score: 3
Revisions (0)
No revisions yet.