patternjavaMinor
TicTacToe - introduction to MVC pattern
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
Please criticize my code, along with its MVC pattern usage.
TicTacToeActivity.java
GameController.java
GameControllerImpl.java
GameControllerAndroidImpl.java
GameView.java
GameViewImpl.java
```
public abstract class GameViewImpl implements GameV
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:
Express this as:
In the following:
The
This naturally leads to a single return statement:
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:
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:
These can also be written as:
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:
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
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.