Loading ...
Sorry, an error occurred while loading the content.

Refactoring Challenge, Part 1

Expand Messages
  • Bill Wake
    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?
    • Charlie Poole
      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.