Tuesday, June 18, 2013

TDDing yourself into a corner

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?

2 comments:

  1. 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.

    I 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().



    ReplyDelete
    Replies
    1. 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.

      Delete