## Re: [XP] Re: Duplication vs Intention Revealing code (was: Zen Refactoring)

Expand Messages
• From: yahoogroups@jhrothjr.com To: extremeprogramming@yahoogroups.com
Message 1 of 42 , Jun 1, 2005
• 0 Attachment
From: "yahoogroups@..."
<yahoogroups.at.jhrothjr.com@...>
To: "extremeprogramming@yahoogroups.com"
<extremeprogramming.at.yahoogroups.com@...>
Sent: Wednesday, June 01, 2005 7:55 AM
Subject: Re: [XP] Re: Duplication vs Intention Revealing code (was: Zen
Refactoring)

And here's the rest of the project, adding in the hundreds
and thousands. Frankly, I found the final result to be,
well, flabbergasting. I had no idea that it would turn out
that pretty.

-----------------------------------------------------

This is where it stood when I ended for the night,
and before I added the hundreds routine. On reflection,
I took a wrong turn there, so I hit the virtual undo
button and backed out to this solution.

class ConvertNumbersToEnglish:
def __init__(self):
for i in range(10):
if tensArray[i] == None:
tensArray[i] = digitArray[i]

tensArray = ["", "ten", "twen", "thir", None, None,
None, None, "eigh", None]

digitArray = ["zero", "one", "two", "three", "four",
"five", "six", "seven", "eight", "nine"]

def numberToEnglish(self, number):
tens, digits = divmod(number, 10)
if tens == 0:
return self.digitArray[digits]
elif tens == 1:
if digits < 3:
return self.teenArray[digits]
else:
return "%steen" % self.tensArray[digits]
elif digits == 0:
return "%sty" % tensArray[tens]
else:
return "%sty-%s" % (self.tensArray[tens],
self.digitArray[digits])

teenArray = ["ten", "eleven", "twelve"]

-------------------------------

It turns out that there are two significant issues here:
five -> fifty and fifteen, not fivety and fiveteen,
and also four -> forty and fourteen, not forteen.

The first is a simple fix to the tens array:

tensArray = ["", "ten", "twen", "thir", None, "fif",
None, None, "eigh", None]

The second is not quite so simple. I'm going to take the
easy way out and extend the teenArray to cover both thirteen
and fourteen as special cases, and simply ignore the duplication
of thirteen (which can be handled by the standard routine) and
of the -teen suffix.

-------------------------------

class ConvertNumbersToEnglish:
def __init__(self):
for i in range(10):
if tensArray[i] is None:
tensArray[i] = digitArray[i]

tensArray = ["", "", "twen", "thir", None, "fif",
None, None, "eigh", None]

digitArray = ["zero", "one", "two", "three", "four",
"five", "six", "seven", "eight", "nine"]

def numberToEnglish(self, number):
tens, digits = divmod(number, 10)
if tens == 0:
return self.digitArray[digits]
elif tens == 1:
if digits < 5:
return self.teenArray[digits]
else:
return "%steen" % self.tensArray[digits]
elif digits == 0:
return "%sty" % tensArray[tens]
else:
return "%sty-%s" % (self.tensArray[tens],
self.digitArray[digits])

teenArray = ["ten", "eleven", "twelve", "thirteen", "fourteen"]

---------------------------------

So let's add hundreds next. After some refactoring,
the result is:

digitArray = ["", "one", "two", "three", "four",
"five", "six", "seven", "eight", "nine"]

def numberToEnglish(self, number):
if number == 0:
return "zero"
high, low = divmod(number, 100)
return (self.getLeftPart(high) + " " +
self.getRightPart(low)).strip()

def getLeftPart(self, number):
if number == 0:
return ""
return self.digitArray[number] + " hundred and"

def getRightPart(self, number):
tens, digits = divmod(number, 10)
if tens == 0:
return self.digitArray[digits]
elif tens == 1:
if digits < 5:
return self.teenArray[digits]
else:
return "%steen" % self.tensArray[digits]
elif digits == 0:
return "%sty" % tensArray[tens]
else:
return "%sty-%s" % (self.tensArray[tens],
self.digitArray[digits])

------------------------------

Notice that I also took zero out of the digitArray: it's
now a special case, reflecting the historical squabble
about whether zero was actually a number a few centuries
back.

The next thing to do is add thousands.

------------------------------

def getLeftPart(self, number):
if number == 0:
return ""
thousands, hundreds = divmod(number, 10)
thousandString = ""
if thousands > 0:
thousandString = self.digitArray[thousands] + " thousand"
hundredString = ""
if hundreds > 0:
hundredString = self.digitArray[hundreds] + " hundred"
comma = ""
if thousandString and hundredString:
comma = ", "
return thousandString + comma + hundredString + " and"

------------------------------

While this works, the comma and 'and' is bugging me. I
think I'll factor that logic out into a separate routine.
I'll also deal with another duplication in the left routine.
The result is:

-----------------------------------

def numberToEnglish(self, number):
if number == 0:
return "zero"
high, low = divmod(number, 100)
return self.combine(self.getLeftPart(high), self.getRightPart(low),
" and ")

def getLeftPart(self, number):
if number == 0:
return ""
thousands, hundreds = divmod(number, 10)
thousandString = self.blankIfZero(thousands, "thousand")
hundredString = self.blankIfZero(hundreds, "hundred")
return self.combine(thousandString, hundredString, ", ")

def blankIfZero(self, digit, suffix):
if not digit:
return ""
return self.digitArray[digit] + " " + suffix

def combine(self, left, right, connector):
if not left:
return right
if not right:
return left
return left + connector + right

---------------------------------------

We're almost there. There is one more special case:
we usually say "thirteen hundred", not "one thousand,
three hundred".

def getLeftPart(self, number):
if number == 0:
return ""
thousands, hundreds = divmod(number, 10)
thousandString = self.blankIfZero(thousands, "thousand")
hundredString = self.blankIfZero(hundreds, "hundred")
if 11 <= number <= 19:
thousandString = ""
hundredString = self.getRightPart(number) + "hundred"
return self.combine(thousandString, hundredString, ", ")

---------------------------------------

So, let's put it all together and see if there's any more duplication
that can be refactored out. I'm also going to clean up getRightPart
a bit: the combination of internal returns and else logic bugs some
people (me included sometimes.)

class ConvertNumbersToEnglish:
def __init__(self):
for i in range(10):
if tensArray[i] is None:
tensArray[i] = digitArray[i]

tensArray = ["", "", "twen", "thir", None, "fif",
None, None, "eigh", None]

teenArray = ["ten", "eleven", "twelve", "thirteen", "fourteen"]

digitArray = ["zero", "one", "two", "three", "four",
"five", "six", "seven", "eight", "nine"]

def numberToEnglish(self, number):
if number == 0:
return "zero"
high, low = divmod(number, 100)
return self.combine(self.getLeftPart(high), self.getRightPart(low),
" and ")

def getLeftPart(self, number):
if number == 0:
return ""
thousands, hundreds = divmod(number, 10)
thousandString = self.blankIfZero(thousands, "thousand")
hundredString = self.blankIfZero(hundreds, "hundred")
if 11 <= number <= 19:
thousandString = ""
hundredString = self.getRightPart(number) + "hundred"
return self.combine(thousandString, hundredString, ", ")

def getRightPart(self, number):
tens, digits = divmod(number, 10)
if tens == 0:
return self.digitArray[digits]
if tens == 1:
if digits < 5:
return self.teenArray[digits]
return self.tensArray[digits] + "teen"
if digits == 0:
return self.tensArray[tens] + "ty"
return self.tensArray[tens] + "ty-" + self.digitArray[digits]

def blankIfZero(self, digit, suffix):
if not digit:
return ""
return self.digitArray[digit] + " " + suffix

def combine(self, left, right, connector):
if not left:
return right
if not right:
return left
return left + connector + right

-------------------------------

getRightPart definitely looks like it can be refactored
using the same routines we developed for getLeftPart.
We have to change blankIfZero a bit, though.
The result is:

def getLeftPart(self, number):
if number == 0:
return ""
thousands, hundreds = divmod(number, 10)
thousandString = self.blankIfZero(thousands, " thousand")
hundredString = self.blankIfZero(hundreds, " hundred")
if 11 <= number <= 19:
thousandString = ""
hundredString = self.getRightPart(number) + " hundred"
return self.combine(thousandString, hundredString, ", ")

def getRightPart(self, number):
tens, digits = divmod(number, 10)
digitString = self.blankIfZero(self.digitArray[digits], "")
tensString = self.blankIfZero(self.tensArray[tens], "ty")
if 11 <= number <= 19:
tensString = ""
if digits < 5:
digitString = self.teenArray[digits]
else:
digitString = self.tensArray[digits] + "teen")
return self.combine(digitString, tensString, "-")

def blankIfZero(self, digit, suffix):
if not digit:
return ""
return self.digitArray[digit] + suffix

------------------------------

Whoah! getLeftPart and getRightPart just jump out as having a
lot of duplication. Without going through the gory details,
here's the result:

-------------------------------

class ConvertNumbersToEnglish:
tensArray = ["", "", "twen", "thir", "for", "fif",
None, None, "eigh", None]

teenArray = ["ten", "eleven", "twelve", None, None,
None, None, None, None, None]

digitArray = ["", "one", "two", "three", "four",
"five", "six", "seven", "eight", "nine"]

self.tensArray[6] = self.digitArray[6]
self.tensArray[7] = self.digitArray[7]
self.tensArray[9] = self.digitArray[9]
self.teenArray[3] = self.tensArray[3]
self.teenArray[4] = self.digitArray[4]
self.teenArray[5:] = self.tensArray[5:]

def numberToEnglish(self, number):
if number == 0:
return "zero"
high, low = divmod(number, 100)
highString = self.getPart(high, digitArray, " thousand", " hundred",
", ")
lowString = self.getPart(low, tensArray, "ty", "", "-")
return self.combine(highString, lowString, " and ")

def getPart(self, number, highArray, highSuffix, lowSuffix,
conjunction):
tens, digits = divmod(number, 10)
digitString = self.blankIfZero(self.digitArray[digits], highSuffix)
tensString = self.blankIfZero(self.highArray[tens], lowSuffix)
if 11 <= number <= 19:
tensString = ""
digitString = self.teenArray[digits]
if digits > 2:
digitString = digitString + "teen"
return self.combine(digitString, tensString, conjunction)

def blankIfZero(self, digit, suffix):
if not digit:
return ""
return self.digitArray[digit] + suffix

def combine(self, left, right, connector):
if not left:
return right
if not right:
return left
return left + connector + right

==========================

That's it. I cleaned up the arrays a bit to
make it obvious what was copied, and where
it comes from.

I'm just gratified with the shape of the final result.

John Roth
• ... One way I ve found to avoid this is to use IntelliJ s awkwardly named Replace Method Code Duplicates refactoring. It looks for all blocks of code that
Message 42 of 42 , Jun 4, 2005
• 0 Attachment
On 6/1/05, Elizabeth Keogh <ekeogh@...> wrote:
> I've extracted a method from a test, then applied the extraction to a
> similar chunk in another test, but missed out one of the parameters which

One way I've found to avoid this is to use IntelliJ's awkwardly named
"Replace Method Code Duplicates" refactoring. It looks for all blocks
of code that have the same structure as the contents of the method,
and replaces them with a call to that method. It's the natural
successor to Extract Method -- perhaps a better name would be "Apply
Method". It's not perfect, but when it works it's a thing of beauty.

Yesterday it saved us from just the problem you mentioned. We tried to
apply it and it *didn't* select the code we thought it would; it
turned out we'd missed a parameter. Once we introduced that parameter
it worked. If we'd been doing this by hand we'd have made your
mistake.

- A
Your message has been successfully submitted and would be delivered to recipients shortly.