HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

TicTacToe - introduction to MVC pattern

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
introductiontictactoepatternmvc

Problem

After reading about MVC pattern in Head First Design Patterns, I've decided to write a TicTacToe app.

I will not reveal the source code of the classes Matrix, Dimension, and Size because they do not relate to the topic. All source code can be found here.

Please criticize my code, along with its MVC pattern usage.

TicTacToeActivity.java

public class TicTacToeActivity extends Activity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        GameModel model = new GameModelImpl(new Dimension(4, 4));
        GameController controller = new GameControllerAndroidImpl(model, this);
    }
}


GameController.java

public interface GameController {
    void onViewIsReadyToStartGame();
    void onPlayerMove(Matrix.Position movePos);
}


GameControllerImpl.java

abstract class GameControllerImpl implements GameController {

    private final GameModel model;

    public GameControllerImpl(GameModel model) {
        this.model = model;
    }

    protected abstract GameView getGameView();

    @Override
    public void onViewIsReadyToStartGame() {
        model.onViewIsReadyToStartGame();
    }

    @Override
    public void onPlayerMove(Matrix.Position movePos) {
        getGameView().blockMoves();
        model.onPlayerTurn(movePos);
        getGameView().unblockMoves();
    }
}


GameControllerAndroidImpl.java

public class GameControllerAndroidImpl extends GameControllerImpl {

    private final GameView gameView;

    public GameControllerAndroidImpl(GameModel model, Activity activity) {
        super(model);
        gameView = new GameViewAndroidImpl(this, model, activity);
    }

    @Override
    protected GameView getGameView() {
        return gameView;
    }
}


GameView.java

public interface GameView {
    void blockMoves();
    void unblockMoves();
    boolean movesBlocked();
}


GameViewImpl.java

```
public abstract class GameViewImpl implements GameV

Solution

Note that the following line violates the principle of information hiding:

if (gameBoard.get(row, column) == Cell.EMPTY) {


Express this as:

if (gameBoard.isEmpty(row, column)) {


In the following:

if (rowResultInfo.resultIsKnown()) {
        return rowResultInfo;
    } else {
        return columnResultInfo(index);
    }


The else is superfluous:

if (rowResultInfo.resultIsKnown()) {
        return rowResultInfo;
    }

    return columnResultInfo(index);


This naturally leads to a single return statement:

return rowResultInfo.resultIsKnown() ? rowResultInfo : columnResultInfo(index);


Much of the code breaks encapsulation, and very little of the code is extensible. I'd recommend you read about self-encapsulation to see how to develop code that is extensible (i.e., subscribes to the Open-Closed Principle).

A lot of information is passed between various classes that incurs duplication. For example, all of these are a form of repetition:

gameBoard.get(pos) == Cell.EMPTY
if (gameBoard.get(row, column) == Cell.EMPTY) {
if (firstCellOnLine == Cell.EMPTY) {
if (cell == Cell.PLAYER) {
} else if (cell == Cell.OPPONENT) {


These are all examples of thinking about programming in terms of functions, rather than in terms of objects. Knowledge of an object's state should stay as close to the object as possible. In the cases above, there is no reason to expose the inner workings of the cell. The first step to hiding the cell's state is by eliminating its get accessor method:

if( cell.isTakenBy( Cell.EMPTY ) )
if( cell.isTakenBy( Cell.OPPONENT ) ) 
if( cell.isTakenBy( Cell.PLAYER ) )


These can also be written as:

if( cell.isEmpty() )
if( cell.isOpponent() ) 
if( cell.isPlayer() )


I get the feeling that there should be no distinction between an "opponent" and a "player" -- there should be only players, one being the "active" player. This would allow for variations such as 4-player T-T-T. You could then write, for example:

if( cell.isTakenBy( currentPlayerToken ) )
if( !cell.isTakenBy( currentPlayerToken ) && !cell.isEmpty() )


For little effort with this approach, the game can use tokens to occupy board positions, rather than a "player" being in a cell. (That is, when playing TTT, the players themselves aren't actually occupying different spaces in the grid, rather their tokens are -- in this case an X or an O. With the model as developed, it strongly implies that a PLAYER or an OPPONENT is in a cell, which, frankly, is nonsensical for TTT, but would make sense for other games where the player physically occupies the board space.)

By making the design a little more generic, it can relatively easily apply to more games.

Note that IconRandomizer isn't a "class" per se. A class, strictly speaking, must have both attributes and behaviour.

Code Snippets

if (gameBoard.get(row, column) == Cell.EMPTY) {
if (gameBoard.isEmpty(row, column)) {
if (rowResultInfo.resultIsKnown()) {
        return rowResultInfo;
    } else {
        return columnResultInfo(index);
    }
if (rowResultInfo.resultIsKnown()) {
        return rowResultInfo;
    }

    return columnResultInfo(index);
return rowResultInfo.resultIsKnown() ? rowResultInfo : columnResultInfo(index);

Context

StackExchange Code Review Q#29070, answer score: 2

Revisions (0)

No revisions yet.