patternjavascriptMinor
Converting from decimals to roman numerals
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.
The
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.orgSolution
-
I would define a constant
-
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:
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.