HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Number to Roman in Javascript

Submitted by: @import:stackexchange-codereview··
0
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

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.