About the problems of using the new operator inside a class that'll be Unit-Tested

Question

Intro

Currently I have something of the form:

Tetris class ---> FallingPiece class ----> Piece class

A Piece can be a Square, a T, etc. It has info about its shape and the shapes of its rotations, its size, etc.

The FallingPiece class basically contains an attribute with a reference to a Piece (the currently falling piece in the Tetris game) and will probably have its current (x, y) location and its color.

My initial design was to have something of the form:

class Tetris {
    private IPieceGenerator pieceGenerator;
    private FallingPiece fallingPiece;
    ...

    public Tetris(IPieceGenerator pieceGenerator) {
        this.pieceGenerator = pieceGenerator;
        ...
    }

    private void someMethodThatNeedsAFallingPiece() {
        if (fallingPiece == null) {
            Piece piece = pieceGenerator.Generate();
            fallingPiece = new FallingPiece(piece);
        }

        ...
    }
}

which of course has the problem that if I later want to Unit-Test my Tetris class and to know in which (x, y) location of the board my current FallingPiece is, I can't. I remember seeing this "problem" exposed in the fabulous Misko Hevery's The Clean Code Talks .

The first problem

seems to be that as I'm giving the Tetris class the responsability of creating FallingPiece objects, I can't then have a reference to them (I have passed through constructor injection a Piece Factory, not FallingPiece Factory!).

Now, I can see at least 2 ways to solve this:

1. I could just define an internal(C#)/package-protected(Java) getter for the FallingPiece attribute, so I can easily test it. This might seem harmless, but I don't find it too elegant.
2. I could instead of passing it a Piece Factory, pass a FallingPiece Factory. I could then control which objects would be returned by the Factory and access them through it. What do you guys think of this way? Is it commonly used?

Is there any other way of solving this?

There is a second problem

related to fact that I've originally implemented a FallingPiece to be an immutable type. This means that every time I want to update the FallingPiece's position on the Tetris board, for example, I'll have to create a new FallingPiece instance and the attribute on Tetris will now point to a new FallingPiece. Which can be a big problem if I want, for example, to have access to the FallingPiece reference through a FallingPieceFactory that is passed to the Tetris class. It appears to me that immutable data types can be if misused a lot of headache when trying to test classes, right? Or is this a bad use of an immutable datatype, in the first place?

Thanks

Answer 1

Possiblle solution:

1. Instead of wiring implementations together, use interfaces.
2. Intruduce a generator for creating FallingPiece

This way you can test everything using mock objects for your interfaces. change visibility of someMethodThatNeedsAFallingPiece to protected to access it in your test case or call it using reflection (evil).

public class Tetris {

    private IPieceGenerator pieceGenerator;
    private IFallingPiece fallingPiece;
    private IFallingPieceGenerator fallingPieceGenerator;

    public Tetris(IPieceGenerator pieceGenerator, IFallingPieceGenerator fallingPieceGenerator) {
      this.pieceGenerator = pieceGenerator;
      this.fallingPieceGenerator = fallingPieceGenerator;

    }

    protected void someMethodThatNeedsAFallingPiece() {
      if (fallingPiece == null) {
          IPiece piece = pieceGenerator.Generate();
          fallingPiece = fallingPieceGenerator.generate(piece);
      }

    }

}

Here's your unit test:

public class TetrisTest {

    private IPieceGenerator pieceGeneratorMock;

    private IFallingPieceGenerator fallingPieceGeneratorMock;

    private IFallingPiece fallingPieceMock;

    private IPiece pieceMock;

    private Tetris tetris;



    @Before
    public void init() {
      this.pieceGeneratorMock = EasyMock.createMock(IPieceGenerator.class);
      this.fallingPieceGeneratorMock = EasyMock.createMock(IFallingPieceGenerator.class);
      this.fallingPieceMock = EasyMock.createMock(IFallingPiece.class);
      this.pieceMock = EasyMock.createMock(IPiece.class);
      this.tetris = new Tetris(pieceGeneratorMock, fallingPieceGeneratorMock);



    }

    @Test
    public void testSomeMethodThatNeedsAFallingPiece() {
      expect(pieceGeneratorMock.Generate()).andReturn(pieceMock);
      expect(fallingPieceGeneratorMock.generate(pieceMock)).andReturn(fallingPieceMock);
      replay(fallingPieceMock, fallingPieceGeneratorMock, pieceGeneratorMock, pieceMock);

      tetris.someMethodThatNeedsAFallingPiece();

      verify(fallingPieceMock, fallingPieceGeneratorMock, pieceGeneratorMock, pieceMock);

    }

}

Answer 2

I would inject only a FallingPieceFactory to reduce the coupling of Tetris to other classes. It looks like that it havn't to know about the PieceFactory AND the FallingPieceFactory.

I would orientate the testing on the usage of the Tetris class. Your method must have an observable effect. How is it used? Is there a class that uses the data of Tetris to paint the game field? Then there would be a getter anyway. Or is the painting called by the Tetris class? Then it have to had a reference to the responsible class and you can inject and observe (possible mock) this painting class.

Regarding immutibility: I never experienced any problems with testing immutable objects. If FallingPiece is a good candidate to be immutable seems questionable as the main purpose seems to be fall down (means: change its position).

Answer 3

I would say that the Tetris object is doing to much:

protected void someMethodThatNeedsAFallingPiece() {
  if (fallingPiece == null) {
      IPiece piece = pieceGenerator.Generate();
      fallingPiece = fallingPieceGenerator.generate(piece);
  }
}

This method has too many reponsibilities.

1. Logic to validate non-null fallingPiece.
2. Generate piece.
3. Generate new fallingPiece.

You should be following the 'Last Possible Moment of Decision`. Get rid of all that. KISS.

Try this:

protected void someMethodThatNeedsAFallingPiece() {
  fallingPiece = pieceModel.getFallingPiece();
}

Push that logic into the pieceModel