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

Converting from decimals to roman numerals

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
decimalsromannumeralsconvertingfrom

Problem

I've submitted the following code for a job interview, and I wanna know if there is anything I can improve on it, or better ways to do the same thing.

const algarismsMap = {
    1 : {
        1 : 'I',
        4 : 'IV',
        5 : 'V',
        9 : 'IX'
    },
    1e1 : {
        1 : 'X',
        4 : 'XL',
        5 : 'L',
        9 : 'XC'
    },
    1e2 : {
        1 : 'C',
        4 : 'CD',
        5 : 'D',
        9 : 'CM'
    },
    1e3: {
        1 : "I\u0305",
        4 : 'I\u0305V\u0305',
        5 : 'V\u0305',
        9 : 'I\u0305X\u0305'
    },
    1e4: {
        1 : "X\u0305",
        4 : 'X\u0305L\u0305',
        5 : 'L\u0305',
        9 : 'X\u0305C\u0305'
    },
    1e5 : {
        1 : "C\u0305",
        4 : 'C\u0305D\u0305',
        5 : 'D\u0305',
        9 : 'C\u0305M\u0305'
    }
}

const divisors = Object.keys( algarismsMap ).reverse()

const romanizeNumber = module.exports = function( n ) {

    // not throwing an error because this can happen while recursing
    if ( n = 4e6 ) {
        throw new Error( 'The max supported number to be converted is 3999999' )
    }

    let romanizedNumber = ''

    // Some special cases for the M algarism.
    if ( n >= 1e3 && n = 1e6 && n = 1e6 ? 'M\u0305' : 'M' ).repeat( n / (n >= 1e6 ? 1e6 : 1e3 ) )
        romanizedNumber += romanizeNumber( n % (n >= 1e6 ? 1e6 : 1e3 ) )

    } else {

        for ( let i = 0; i = currInternalDivisor * currDivisor ) {

                    // the 1 check here is basically for the same motive than the 'M' check above.
                    romanizedNumber += currInternalDivisor === 1 ? currInternalDivisorAlgarism.repeat( n / currDivisor ) : currInternalDivisorAlgarism

                    romanizedNumber += romanizeNumber( n % (currInternalDivisor * currDivisor) )

                    return romanizedNumber
                }
            }
        }

    }

    return romanizedNumber
}


The \u0305 is the unicode for the combining overline character: https://en.wikipedia.org

Solution

-
I would define a constant overline="\u0305" and then use interpolation further down: 4 : C${overline}D#{overline},, it might even make sense to define overlineD and overlineC constants. An alternative might be to create an overline method and write something like 4: overline('CD') or even a tagged template literal (4: overlineCD`) if you want to impress.

-
Object.keys( algarismsMap ).reverse(): The order of keys in a hash is not guaranteed, it can vary depending on the implementation and if keys have been deleted, etc. If you need order you should use an array. Also rather than reversing just define it backwards.

-
Rather than
if {...} else [lots of code] I find it better to use an early return if {...; return; } [lots of code]`

-
One of the biggest thing to me is the lack of tests. I would expect to see something in an interview assignment, it doesn't have to use a framework, I would be fine with something like:

function test(number, roman) {
  if (romanizeNumber(number) !== roman)
    console.error(`Failed! ${number} should be ${roman} but was ${romanizeNumber(number)}`);
}
test(10, 'X');
test(11, 'XI');
...

Code Snippets

function test(number, roman) {
  if (romanizeNumber(number) !== roman)
    console.error(`Failed! ${number} should be ${roman} but was ${romanizeNumber(number)}`);
}
test(10, 'X');
test(11, 'XI');
...

Context

StackExchange Code Review Q#162521, answer score: 3

Revisions (0)

No revisions yet.