The next tests are for what what happens from 4 onwards.
Convert 4 to IV
test("4 converts to IV", function () {
expect(convert(4)).toBe("IV");
});
We can make that pass by testing for 4 and returning it before the ones.
if (n === 4) {
return "IV";
}
return convert(n - 1) + "I";
The tests are showing quite some duplication now. We should be able to use an array to hold the values being converted, and loop through them to perform the tests.
let convertedValues = [
"I", "II", "III", "IV"
];
convertedValues.forEach(function (roman, index) {
const arabic = index + 1;
test(`${arabic} converts to ${roman}`, function () {
expect(convert(arabic)).toBe(roman);
});
});
While that works, I don’t like how I have to convert the index number to an arabic value.
I don’t want to include 0 in there either, because it doesn’t belong as a roman value.
The following test code would work wonderfully:
let convertedValues = [
{"1": "I"},
{"2": "II"},
{"3": "III"},
{"4": "IV"}
];
Object.entries(convertedValues).forEach(function ([arabic, roman]) {
test(`${arabic} converts to ${roman}`, function () {
expect(convert(arabic)).toBe(roman);
});
});
But, Jest doesn’t understand the ES6 Object.entries() code.
So instead, another alternative is to use arrays instead of objects, which gives us:
let convertedValues = [
[1, "I"],
[2, "II"],
[3, "III"],
[4, "IV"]
];
convertedValues.forEach(function ([arabic, roman]) {
test(`${arabic} converts to ${roman}`, function () {
expect(convert(arabic)).toBe(roman);
});
});
This way, we can just add move values to the array, for each new test.
So now that the tests are all passing, we need to commit this update:
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Refactor the tests to use an array for each conversion test"
Convert 5 to V
It’s now easier to add the test for this conversion. Below the number 4 test, we just add another test for 5.
[4, "IV"],
[5, "V"]
Making this pass can be done by adding an if statement to the code:
if (n === 5) {
return "V";
}
return convert(n - 1) + "I";
But where should it go? It works both above and below the one that checks for number 4.
Never mind, the code works so commit, and then considering this can be done when refactoring.
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Convert 5 to V"
Before refactoring, I’ll wait for a third if statement, which should be when we get to number 10.
The tests for 6 through to 8 all pass, with the tests just being:
[5, "V"],
[6, "VI"],
[7, "VII"],
[8, "VIII"]
Those are all worth committing too.
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Tests for converting 6 through to 8 all pass without needing to change the code"
Convert 9 to IX
The number 9 test is easy to add to the array in the test code:
[8, "VIII"],
[9, "IX"]
We can add a new if condition for 9:
if (n === 9) {
return "IX";
}
return convert(n - 1) + "I";
And all the tests pass. Commit the code.
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Convert 9 to IX"
Is the code worth refactoring yet? I think that we’ll really see how it should be refactored when we get to 10.
Convert 10 to X
The test to add for this is added below the 9 test:
[9, "IX"],
[10, "X"]
And I thought that the code to make this pass would be another if statement:
if (n === 10) {
return "X";
}
return convert(n - 1) + "I";
But the code doesn’t pass. Instead of getting "X" I get "IXI", so that if statement for 10 needs to come before the one for 9.
if (n === 10) {
return "X";
}
if (n === 9) {
return "IX";
}
And the test passes. I should make a similar change to the other if statements too, with the highest values coming before the lower values:
if (n === 10) {
return "X";
}
if (n === 9) {
return "IX";
}
if (n === 5) {
return "V";
}
if (n === 4) {
return "IV";
}
return convert(n - 1) + "I";
And now it’s a good time to commit this working code:
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Convert 10 to X"
We can now focus on refactoring the code, to remove some of that duplication.
A few possibilities exist for reducing the number of if statements. We could check if the value of the next roman numeral is larger, and take certain steps. But, that it likely to get very messy when dealing with L, C, D, and M.
As with the tests, I do like the idea of having the information in an array.
var CONVERSION_FACTORS = [
[10, "X"],
[9, "IX"],
[5, "V"],
[4, "IV"]
];
And the code can then get the roman characters from that array.
if (n === 10) {
return CONVERSION_FACTORS[0][1];
}
if (n === 9) {
return CONVERSION_FACTORS[1][1];
}
if (n === 5) {
return CONVERSION_FACTORS[2][1];
}
if (n === 4) {
return CONVERSION_FACTORS[3][1];
}
And that works, all the tests pass, but is it an improvement?
I could search the CONVERSION_FACTORS array for the first item that’s greater or equal to n, that might work well.
if (n === 10) {
return CONVERSION_FACTORS.find(function ([arabic, ignore]) {
return (arabic >= n);
}).pop();
}
Yes, that works well. I don’t want to add all of that to the other tests though.
Can I use that for other tests, such as with 9 as well?
if (n >= 9) {
return CONVERSION_FACTORS.find(function ([arabic, ignore]) {
return (arabic >= n);
}).pop();
}
No, the test fails. It gets X instead of IX. If the array was reversed, then we might really have something.
var CONVERSION_FACTORS = [
[4, "IV"],
[5, "V"],
[9, "IX"],
[10, "X"]
];
if (n >= 9) {
return CONVERSION_FACTORS.find(function ([arabic, ignore]) {
return (arabic >= n);
}).pop();
}
if (n === 5) {
return CONVERSION_FACTORS[1][1];
}
if (n === 4) {
return CONVERSION_FACTORS[0][1];
}
Yes, that works. It’s time to commit this while we’re ahead, because I don’t want too much time to pass between commits, and things are currently all working.
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Partway through refactoring so that a CONVERSION_FACTORS array is used"
With that committed, I feel better about moving on to finish this refactoring.
Improving the conversion
When I try to lower the if condition to 8, the code doesn’t work, getting IX instead of VIII
The comparison is wrong inside of the find statement. It’s getting higher roman values than it should.
Returning the if condition back up to 9, I can flip >= to <= which corrects that problem, but the tests still don’t pass.
I need to flip the array back around to how it began, with high values at the top.
var CONVERSION_FACTORS = [
[10, "X"],
[9, "IX"],
[5, "V"],
[4, "IV"]
];
// ...
if (n === 5) {
return CONVERSION_FACTORS[2][1];
}
if (n === 4) {
return CONVERSION_FACTORS[3][1];
}
and then -
No, I don’t go looking at 8. I’ve fixed a problem in the code and all tests pass, so before exploring further, I commit that fix.
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Using correct <= comparison to get roman characters within range of n"
Getting 8 and lower working with the find function
Changing the if statement to (n >= 8) I can see now from the test, that it’s getting “V” instead of “VIII”
I just need to subtract the arabic value and return the roman character along with another call to the convert function.
To do that, I need to have a separate if statement inside of the find function.
// return (arabic <= n);
if (arabic <= n) {
return true;
}
I can now subtract the arabic value when inside of that if condition, and add on the converted value of what remains afterwards:
return CONVERSION_FACTORS.find(function ([arabic, ignore]) {
if (arabic <= n) {
n -= arabic;
return true;
}
}).pop() + convert(n);
That works. and lowering the if condition down to 4, it still passes all the tests.
Going down to 3 though it doesn’t work, because it can’t find anything in the array.
Can I deal with that by adding “I” to the array?
[4, "IV"],
[1, "I"]
Yes that works. Can I go all the way now, and remove the if statement completely?
// if (n >= 3) {
return CONVERSION_FACTORS.find(function ([arabic, ignore]) {
if (arabic <= n) {
n -= arabic;
return true;
}
}).pop() + convert(n);
// }
Hey yes that works! Let’s remove the other code that was below it:
// if (n === 5) {
// return CONVERSION_FACTORS[2][1];
// }
// if (n === 4) {
// return CONVERSION_FACTORS[3][1];
// }
// return convert(n - 1) + "I";
And it looks like we now have a working algorithm. Time to commit this quick.
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Using CONVERSION_FACTORS for all of the conversions now, instead of separate if statements"
Finishing off the tests
Based on the IV, V and IX X pattern, we can finish off the rest of the tests in a rather rapid order, for 40 and 50, 90 and 100, 400 and 500, and 1000. One at a time though, ending up with test code of:
// ...
[10, "X"],
[40, "XL"],
[50, "L"],
[90, "XC"],
[100, "C"],
[400, "CD"],
[500, "D"],
[1000, "M"]
And the following code that makes it pass:
var CONVERSION_FACTORS = [
[1000, "M"],
[500, "D"],
[400, "CD"],
[100, "C"],
[90, "XC"],
[50, "L"],
[40, "XL"],
[10, "X"],
// ...
Commit this working code, and there’s only one more thing to go:
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Add remaining tests and code for 40, 50, 90, 100, 400, 500, and 1000"
A final test
To make sure that everything’s working properly, one final test to convert 1999 should help to make sure that everything working correctly. I don’t want to use the output of my program for the converted value, so Roman numerals converter tells me that 1999 converts to MCMXCIX
The test is:
[1999, "MCMXCIX"]
And the code fails. What? It shouldn’t fail.
Expected value to be (using Object.is):
"MCMXCIX"
Received:
"MDCDXCIX"
I went too fast with those tests, and forgot to put in 900.
The tests are:
[900, "CM"],
[1000, "M"],
[1999, "MCMXCIX"]
And the updated code is:
var CONVERSION_FACTORS = [
[1000, "M"],
[900, "CM"],
All 21 tests now pass, so I can commit and push this final lot of code:
> git status
bundle.js
roman-numerals.js
roman-numerals.test.js
> git add *.js
> git commit -m "Added CM to the conversions, and a final check of converting 1999 to MCMXCIX"
> git push
The index page shows all of the roman numerals from 1 to 100, and would do them up to any value we like.
This kata is done, and all of the develop commits commits can now be seen.
Merge develop branch into master
I can now merge this develop branch to master. Before merging though, close the window that’s watching test and build, so that files like bundle.js don’t get updated. It had updated for me after my last commit, so I followed the git status instructions to toss it away:
git checkout -- bundle.js
With the merge, the checkout switches to my local branch, and the pull retrieves any updates from the server
> git checkout master
> git pull origin master
> git merge develop
> git push origin master
> git checkout develop
That last checkout, is because it’s a bad habit to remain on master. Switching back to develop then means that you’r ready to go at any time that you come back to work with this code.
That brings this kata to an end, and the final master commits are now up to date too.