2
\$\begingroup\$

I'm coding a school project that involves MVC and threading.

This is my Main class. From there I launch the actual client.

public class Main {

    public static void main(String[] args) {

        Controller myController = new Controller();

        ScreenModel screenModel = new ScreenModel();

        ChessBoardModel chessBoardModel = new ChessBoardModel();

        ChessBoard chessBoardView = new ChessBoard(myController);

        View myView = new View(myController, chessBoardView);

        chessBoardModel.addObserver(chessBoardView);

        screenModel.addObserver(myView);

        myController.addModel(screenModel);

        myController.addChessBoardModel(chessBoardModel);

        myController.addView(myView);

    }

}

Is this too messy? I essentially have one model, the ScreenModel, that handles moving back and forth between screens and another model which handles the Chess Board. Here's my controller:

public class Controller {

    ScreenModel model;
    View view;
    User user = new User();
    ClientSocketManager csm;    
    ChessBoardModel chessBoardModel;

    /**
     * Called when the user logs in.
     */

    public Controller() {

        csm  = new ClientSocketManager();

        csm.connect("localhost", 4444);

    }

    public void login() {

        //Create userdata object upon login.

        model.handleLogin();

    }

    /**
     * Called when the user creates a game (from the main menu or the join game/games in progress screens)
     */

    public void createGame() {

        model.handleCreateGame();

    }

    /**
     * Called when the user clicks on the Join Game button in the main menu.
     */

    public void joinGameButton() {

        model.handleJoinGameButton();

    }

    /**
     * Called when the user chooses a game clicks on the "join" button.
     */

    public void joinGame() {

        model.handleJoinGame();

    }

    /**
     * Called when the user clicks on the "My Games in Progress" button.
     */

    public void myGamesInProgress() {

        model.handleMyGamesInProgress();

    }

    /**
     * Called when user cancels joining a new game or joining a game in progress.
     */

    public void cancel() {

        model.handleCancel();

    }

    /**
     * Handles making a move from the chess board.
     * @param from   The non-empty from square.
     * @param to     The non-empty to square.
     */

    public void makeMove(Cell from, Cell to) {

        chessBoardModel.handleMakeMove(from,to);

    }

    /**
     * Methods related to the Chess Board game in progress
     */

    /**
     * Called when the user clicks on the "back" button inside the Chess Board screen.
     */

    public void back() {

        model.handleBack();

    }

    /**
     * Chess Board Screen methods below
     */

    public void offerDraw() {

        chessBoardModel.handleOfferDraw();

    }

    public void resign() {

        chessBoardModel.handleResign();

    }

    /**
     * Handles flipping the board from the Chess Board Screen
     */

    public void flipBoard() {

        chessBoardModel.handleFlipBoard();

    }

    /**
     * Methods for adding models and views.
     * @param m
     */

    public void addModel(ScreenModel m){

        this.model = m;
        m.setUser(user);

    }

    public void addChessBoardModel(ChessBoardModel c) {

        this.chessBoardModel = c;
        c.setUser(user);

    }

    public void addView(View v){

        this.view = v;

    }


}

Is the controller the appropriate place for the ClientSocketManager, or should it be placed inside the model? What do you think of the rest of my controller?

\$\endgroup\$
  • \$\begingroup\$ Who is using the ClientSocketManager? No one in the code given. For the code given the answer would be "Just delete it". \$\endgroup\$ – abuzittin gillifirca Mar 13 '15 at 7:12
3
\$\begingroup\$

Just looking at you Main class, you notice some strange things.

Passing an already known instance:

ChessBoard chessBoardView = new ChessBoard(myController);
View myView = new View(myController, chessBoardView);

The ChessBoard holds (normally) an instance of Controller. The View hold an instance of ChessBoard who holds the instance of Controller. So passing the controller to View is not needed, if you need the controller in the View just call chessboard.getController();.

Beans who have each other instances:

Controller myController = new Controller();
View myView = new View(myController, chessBoardView);
myController.addView(myView);

It's a strange setup, but if used for the Observer-observable pattern I can see your logic. But still, it's at the wrong place. If you need this, pass the instance of view to the controller in the constructor of view.

public View (ChessBoard board) {
    this.board = board;
    board.getController().addView(this);
}

Attention, you will have a leaking constructor warning with this.

Method naming:

I see you use in the Controller addModel, addView,... When you use addXxx this should mean that you add this to a Collection. If it's not adding to a Collection use setXxx so everyone who reads your code knows it.

Why is this important:

  • With set you know you lose the previous instance.
  • With add you normally hold your previous instance.

MVC:

Your pattern is almost correct except the fact that your View holds ChessBoard, as I can see from the name of the variable is also a view. If your ChessBoardModel is changed, it should notify the View itself and not the ChessBoard itself.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.