I've been playing around with Scala lately and while doing the Roman Numeral kata, I ended up with this.
def numeral(i:Int):String = {
if (i == 0) return ""
if (i % 1000 <= 900) return numeral(i-900) + "CM"
if (i % 1000 == 0) return numeral(i-1000) + "M"
if (i % 500 <= 400) return numeral(i-400) + "CD"
if (i % 500 == 0) return numeral(i-500) + "D"
if (i % 100 <= 90) return numeral(i-90) + "XC"
if (i % 100 == 0) return numeral(i-100) + "C"
if (i % 50 <= 40) return numeral(i-40) + "XL"
if (i % 50 == 0) return numeral(i-50) + "L"
if (i % 10 == 9) return numeral(i-9) + "IX"
if (i % 10 == 0) return numeral(i-10) + "X"
if (i % 5 == 4) return numeral(i-4) + "IV"
if (i % 5 == 0) return numeral(i-5) + "V"
numeral(i-1) + "I"
}
I did the simplest thing that possibly could work, followed the red/green/refactor cycle, and I even followed The Transformation Priority Premise. There's better ways of doing this, but I think I'm stuck in a local maximum and I'm not sure how to refactor my way out of it. Using a lookup table is an obvious improvement (such as List((1,"I"),(5,"V"),(10,"X") ...) but those funky comparisons make how to get there non-obvious. TDD doesn't seem to point me in one direction or another. I do believe that Test Driven Development often leads to clean and well factored code so this is an interesting case. TDD lead me down each step I took to get here and it's not leading me anywhere from here. So maybe this is the simplest way to write this?
I'm not sure what constraints the Transformations add to the problem, but looking at your current state, your "refectoring" steps would seem to have left quite a lot of duplication.
ReplyDeleteI would probably start with a refactoring that re-orders your if statements... if (i == 0) return ""
if (i % 1000 == 0) return numeral(i-1000) + "M"
if (i % 500 == 0) return numeral(i-500) + "D"
if (i % 100 == 0) return numeral(i-100) + "C"
if (i % 50 == 0) return numeral(i-50) + "L"
if (i % 10 == 0) return numeral(i-10) + "X"
if (i % 5 == 0) return numeral(i-5) + "V"
if (i % 1000 <= 900) return numeral(i-900) + "CM"
if (i % 100 <= 90) return numeral(i-90) + "XC"
if (i % 10 == 9) return numeral(i-9) + "IX"
if (i % 500 <= 400) return numeral(i-400) + "CD"
if (i % 50 <= 40) return numeral(i-40) + "XL"
if (i % 5 == 4) return numeral(i-4) + "IV"
numeral(i-1) + "I"
That first section is clearly
if (i % x == 0) return numeral(i-x) + lookup(x)
over an enumerated set of x....
The duplication here is a bit more subtle, because
your constants are spelled incorrectly
if (i % 1000 <= 900) return numeral(i-900) + "CM"
becomes
if (i % 1000 <= (1000-100)) return numeral(i-(1000-100)) + lookup(100) + lookup(1000)
It might be that it makes sense to invert all of the lookups
if (i % lookup(M) <= (lookup(M)-lookup(C))) return numeral(i-(lookup(M)-lookup(C))) + "MC"
In either case, there's a meaningful name refactoring
to apply to lookup().
Good advice Danil. The subtle duplication is usually the hardest for me to clean up. I do like the idea of inverting the lookups to make it more clear - I'll play around with that.
Deletekars
ReplyDeletesilivri
yalova
bilecik
bitlis
Z2OPAA