Sorry, an error occurred while loading the content.

Refactoring Challenge, Part 1

Expand Messages
• I m a big believer in hands-on fun. Much like the Test-First Challenge of a couple months back, I thought I d create one for refactoring. This is based on a
Message 1 of 38 , May 1, 2002
• 0 Attachment
I'm a big believer in hands-on fun. Much like the "Test-First Challenge" of
a couple months back, I thought I'd create one for refactoring. This is
based on a chapter of the "Refactoring Workbook" that I'm writing.

This and future parts of the challenge will be at
http://www.xp123.com/rwb/ch15-Example/index.htm

Like before, I'd love to hear from you and see what you've done.
(William.Wake @ acm.org)

Thanks!
--
Bill Wake William.Wake@... www.xp123.com

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

Chapter 15: A Recursive Example, Part 1

We'll work through an example involving refactoring, test-driven design, and
recursion.

Suppose we've decided to develop a system to play games in the tic-tac-toe
family: squares occupied by different markers. In tic-tac-toe, you have a
3x3 grid, and try to get three in a row. In Connect Four (trademark Hasbro),
you have a rectangular grid, and try to get four in a row, but columns have
to be filled from bottom to top. We'll start with a simplified version of
tic-tac-toe, and work our way up to the general case.

Here are some tests and the first version of the code:

import junit.framework.*;
public class GameTest extends TestCase {
public GameTest(String s) {super(s);}

public void testDefaultMove() {
Game game = new Game("XOXOX-OXO");
assertEquals(5, game.move('X'));

game = new Game("XOXOXOOX-");
assertEquals(8, game.move('O'));

game = new Game("---------");
assertEquals(0, game.move('X'));

game = new Game("XXXXXXXXX");
assertEquals(-1, game.move('X'));
}

public void testFindWinningMove() {
Game game = new Game("XO-XX-OOX");
assertEquals(5, game.move('X'));
}

public void testWinConditions() {
Game game = new Game("---XXX---");
assertEquals('X', game.winner());
}
}

public class Game {
public StringBuffer board;

public Game(String s) {board = new StringBuffer(s);}

public Game(StringBuffer s, int position, char player) {
board = new StringBuffer();
board.append(s);
board.setCharAt(position, player);
}

public int move(char player) {
for (int i = 0; i < 9; i++) {
if (board.charAt(i) == '-') {
Game game = play(i, player);
if (game.winner() == player)
return i;
}
}

for (int i = 0; i < 9; i++) {
if (board.charAt(i) == '-')
return i;
}
return -1;
}

public Game play(int i, char player) {
return new Game(this.board, i, player);
}

public char winner() {
if (board.charAt(0) != '-'
&& board.charAt(0) == board.charAt(1)
&& board.charAt(1) == board.charAt(2))
return board.charAt(0);

if (board.charAt(3) != '-'
&& board.charAt(3) == board.charAt(4)
&& board.charAt(4) == board.charAt(5))
return board.charAt(3);

if (board.charAt(6) != '-'
&& board.charAt(6) == board.charAt(7)
&& board.charAt(7) == board.charAt(8))
return board.charAt(6);

return '-';
}
}

Notice that the winner() routine is simplified: you win by getting three in
a row horizontally. Notice also that the heuristics for what to play are
primitive: win if you can, play anything otherwise. We'll migrate toward
something capable of more sophisticated strategies.

Exercise:
Go through this code and identify smells.

Exercise:
It's not always easy to know what to do with code. Let's fix some of the
easy things first. Fix them one at a time.
* The name move() isn't descriptive enough. Change it to bestMoveFor().
* The variable i doesn't explain much either. Change it to move.
* The value -1 is a flag value; create a constant NoMove to represent it.
* The winner() function has a lot of duplication. Eliminate the duplication.
* The check for a board character being a '-' is really a check that the
square is unoccupied. Extract a method to do this, and name it
appropriately.

We have two "for" loops: one to find a winning move, the other to find a
default move. One way to handle this would be to extract each one into a
method. As we add more strategies, we could see each strategy getting its
own method. An alternative would be to merge the two loops and handle things
more in one pass through the possible moves. We'll take the latter approach.

Exercise:
We haven't talked about the "Fuse Loops" refactoring. Find a way to merge
these two loops. Do so in small steps, in such a way that you maintain
safety as you do it. When is it safe to merge two loops?

One step along the way might be to assign a value to a temporary variable
rather than return it right away. You might have made the second loop look
like this:
defaultMove = NoMove;
for (int i = 0; i < 9; i++) {
if (board[i] == '-')
if (defaultMove == NoMove)
defaultMove = i;
}

That was the safe approach, used because we did not want our refactoring to
change behavior. To be equivalent to the original, we need the guard clause
to make sure we haven't assigned a defaultMove yet. But let's put on a
development hat: we don't really care which default move we make, so we
could delete the "defaultMove==NoMove" test. It's not necessary to stop when
we find a viable move. (That is, there's no harm in trying each possible
move provided we prefer wins to defaults.) So you can delete the "break"
tests that exit early. Run the tests again and be sure you haven't changed
anything important.

Now we have a single loop, but the condition to decide what to return is
still a little complicated:
if (winningMove != NoMove)
return winningMove;
if (defaultMove != NoMove)
return defaultMove;
return NoMove;

Exercise:
What refactorings would you tackle next?
• Randy, Ah... I was having a flashback to an earlier thread in which someone said that the ternary operator was the same as if... then... else... I see now
Message 38 of 38 , May 8, 2002
• 0 Attachment
Randy,

Ah... I was having a flashback to an earlier thread in which someone
said that the ternary operator was "the same as" if... then... else...

I see now you weren't really saying that.

Charlie Poole
cpoole@...

> (not caring if I don't appreciate the humour.)
>
> Please do not interpret "if" "then" and "else" as the use of
> those words in the actual code.
>
> In this case, please interpret:
>
> "if" as the condition,
> "then" as the expression evaluated if the condition is true;
> "else" as the expression evaluated if the condition is false;
>
> At 03:08 PM 5/2/02 -0700, you wrote:
> >Randy,
> >
> > > I'll agree that, whenever I look at the C ? operator construct, I have
> > > to revisit the manual, only to find it is if?then:else and I
> wonder why
> > > I never remember that.
> >
> >Almost so. You can't substitute if...then...else everywhere that ?:
> >occurs. Also, you can't substitute ?: without change for if...then...else
> >
> >Otherwise, they're exactly the same. :-)
> >
> >Charlie Poole
> >cpoole@...
> >
> >
> >To Post a message, send it to: extremeprogramming@...
> >
> >To Unsubscribe, send a blank message to:
> >extremeprogramming-unsubscribe@...
> >
> >ad-free courtesy of objectmentor.com
> >
> >Your use of Yahoo! Groups is subject to http://docs.yahoo.com/info/terms/
>
> Later...
> -----------------------------------------------------------------------
> |\/| Randy A MacDonald | you can't pay for it,
> |\\| randy@... | even if you want to.
> BSc(Math) UNBF'83 Sapere Aude | APL: If you can say it, it's done..
> Natural Born APL'er |
> ------------------------------------------------------------{ gnat }-
>
>
> To Post a message, send it to: extremeprogramming@...
>
> To Unsubscribe, send a blank message to:
> extremeprogramming-unsubscribe@...
>
> ad-free courtesy of objectmentor.com
>
> Your use of Yahoo! Groups is subject to http://docs.yahoo.com/info/terms/
>
>
>
Your message has been successfully submitted and would be delivered to recipients shortly.