Refactoring Examples: Duplicated Code

I’ve spent the last couple weeks working through Martin Fowler’s Refactoring, which includes a taxonomy of useful solutions to common code smells. It’s an excellent book, but I found it hard to recognize and remember some of the patterns without digging into my own code. Over the next few posts, I’ll describe some of the smells and refactors I found in my Tic Tac Toe game (as much to commit them to my own memory as to share them with others). First up, the most common code smell of all: duplication.

Despite using a superclass to store static methods and data used by all my tests, I wasn’t using inheritance to create universal setup and teardown methods. Although many of my tests used a similar pattern to capture output, none were quite alike, and all of them were pretty messy. Here’s an example:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46

@RunWith(JUnit4.class)
public class GameControllerTest {

    private MockTerminalView view = new MockTerminalView();
    private GameController controller = new GameController(view);

    private final PrintStream stdout = System.out;
    private final ByteArrayOutputStream output = new ByteArrayOutputStream();
    private PrintStream outputStream;
    private OutputRecorder outputRecorder;
  
    @Before
    public void setUp() throws Exception {
        loadViewStrings();
        outputStream = new PrintStream(output, true, "UTF-8");
        outputRecorder = new OutputRecorder(output, true, "UTF-8");
        Player playerOne = new HumanPlayer(X);
        Player playerTwo = new MinimaxPlayer(O);
        controller.setPlayerOne(playerOne);
        controller.setPlayerTwo(playerTwo);
    }

    @Test
    public void controllerShouldStartNewGame() {
        System.setOut(outputRecorder);

        controller.newGame();

        assertEquals(welcome, outputRecorder.popFirstOutput());
        assertEquals(divider, outputRecorder.popFirstOutput());

        System.setOut(stdout);
    }

    @Test(expected = GameOverException.class)
    public void controllerShouldEndGameOnRestartIfInputIsNo() throws GameOverException {
        System.setOut(outputStream);

        view.enqueueInput("n");

        controller.restartGame();
        assertEquals(playAgain, output.toString());

        System.setOut(stdout);
    }
} 

These tests use both a plain PrintStream and my custom OutputRecorder, though they don’t really need to. Some setup is done in the fields, and some during setUp(), though there’s no clear reason why. And the same lines setting up and tearing down the recorder are repeated across lots of tests. The smell here is duplicated code, which Fowler calls “number one in the stink parade.” To solve it, I’ll start by extracting a method, and then extracting a superclass.

First, I’ll create an empty TicTacToeTest class with empty setup and teardown methods.

1
2
3
4
5
6
7
8
9
10
11

@RunWith(JUnit4.class)
public class TicTacToeTest {

  @Before
  public void setUp() {
  }

  @After
  public void cleanUp() {
  }
}

Now, extract a setUpRecorder() method including all the setup-related lines:

1
2
3
4
5
6
7
8

            private OutputRecorder recorder;

            private void setUpRecorder() throws UnsupportedEncodingException {
                    PrintStream stdout = System.out;
                    ByteArrayOutputStream output = new ByteArrayOutputStream();
                    recorder = new OutputRecorder(output, true, "UTF-8");
            }

Then, a startRecorder() method to replace calls to System.setOut():

1
2
3
4

            private void startRecorder() {
              System.setOut(recorder);
              }

This method might seem short, but I find startRecorder() much easier to read. Now that these methods are implemented, I can plug them into the tests:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24

              @Before
              public void setUp() throws Exception {
                  loadViewStrings();
                  
                  setUpRecorder();
                  
                  Player playerOne = new HumanPlayer(X);
                  Player playerTwo = new MinimaxPlayer(O);
                  controller.setPlayerOne(playerOne);
                  controller.setPlayerTwo(playerTwo);
              }

              @Test
              public void controllerShouldStartNewGame() {
                  startRecorder();

                      controller.newGame();

                      assertEquals(welcome, outputRecorder.popFirstOutput());
                      assertEquals(divider, outputRecorder.popFirstOutput());

                      System.setOut(stdout);
                  }

The last line of the second test was meant as a mini-teardown, to reset stdout before the next test. I wrote it before I understood JUnit execution, in which each test runs in its own environment. It survived around 80 commits, but it doesn’t actually do anything, so I can safely delete it. (I know because none of the tests fail afterwards). In fact, this whole refactor should be possible while keeping the tests green.

After searching for usages of the old pattern and replacing them with the new methods, there’s just one thing left: pull up the new methods to a Test superclass. Here’s the result:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20

                  public class TicTacToeTest {

                      protected OutputRecorder recorder;

                      protected void setUpRecorder() throws UnsupportedEncodingException {
                          ByteArrayOutputStream output = new ByteArrayOutputStream();
                          recorder = new OutputRecorder(output, true, "UTF-8");
                      }

                      protected void startRecorder() {
                          System.setOut(recorder);
                      }

                      @Before
                      public void recorderSetUp() throws UnsupportedEncodingException {
                          setUpRecorder();
                      }

                  }

This looks like a simple refactor, but my tests are now much cleaner. More important, if I need to make a change to the OutputRecorder class, it will propagate through all tests.

Comments