Problem
I’m working on a moderately complicated (not super basic, but not a feat of programming strength) project to create a virtual tabletop for a tabletop game I play. I’m trying to conform to good design practices and patterns, specifically I’d like to keep everything as nicely MVC-oriented as possible. My problem is, I’m not certain how to nicely communicate between my view and my controller, or even my view and my components.
My view is made of custom components. Theoretically, each component would have an action/key/button handler based on its unique function, which would report to the view that an action/key/buttonevent had been fired. The View would then convert that event to something more generic (MoveEvent, AttackEvent, etc) and fire said event to the controller.
The current implementation is that the ui handlers have a reference to the view, that the view is an Observable object, and that the controller is an Observer. If I click a command button, the CommandHandler catches the event and notifies the view’s observers that said event has happened.
My problem is twofold:
-
my implementation of the handlers feels wrong. I don’t think I should be passing a reference to View to them, but at the same time it seems like the only other object is to make View itself the listener for all the UI items.
-
using the observer pattern for a single observer>observee relation feels wrong. I can’t put my finger quite on what feels so wrong about it, but it seems like there should be a more specialized interface for message passing that keeps the decoupled nature. Do action listeners/handlers use the Observer pattern or a different method of message passing?
Here’s the code (feel free to point out anything I’m doing wrong that I’m not specifically addressing in these questions as well):
View.java (you can mostly just skim this, lame ui-hand-coding except for the bits at the bottom)
public class View extends Observable {
private MainWindow main;
private JPanel rootPanel, actorPanel, gamePanel, fillerPanel;
private ShipPane actorPane;
private JScrollPane gridHolder, output;
private CommandPanel commandPanel;
private LogPanel logText;
private GridPanel grid;
private Observer obs;
public View() {
main = new MainWindow();
Border blackLineBorder = BorderFactory.createLineBorder(Color.black);
rootPanel = new JPanel(new BorderLayout());
actorPanel = new JPanel(new BorderLayout());
gamePanel = new JPanel(new BorderLayout());
rootPanel.setBorder(blackLineBorder);
actorPane = new ShipPane();
actorPane.setBorder(blackLineBorder);
actorPanel.add(actorPane, BorderLayout.CENTER);
commandPanel = new CommandPanel();
commandPanel.addHandler(new CommandHandler(this)); //RELEVANT
actorPanel.add(commandPanel, BorderLayout.SOUTH);
rootPanel.add(actorPanel, BorderLayout.WEST);
gridHolder = new JScrollPane();
grid = new GridPanel();
grid.setBorder(blackLineBorder);
gridHolder.setViewportView(grid);
gamePanel.add(gridHolder, BorderLayout.CENTER);
output = new JScrollPane();
logText = new LogPanel();
//logText.setMinimumSize(new Dimension(100, 200));
output.setViewportView(logText);
output.setPreferredSize(new Dimension(main.getWidth() - 200, 200));
gamePanel.add(output, BorderLayout.SOUTH);
rootPanel.add(gamePanel, BorderLayout.CENTER);
main.setContentPane(rootPanel);
main.pack();
main.setVisible(true);
}
//EVERYTHING BELOW THIS LINE IS SORTA RELEVANT
public void setActorFocus(Actor a) {
actorPane.populate(a);
}
public void writeToLog(String str) {
logText.writeLine(str);
}
public void addObserver(Observer o) {
obs = o;
}
public Observer getObserver() {
return obs;
}
public class CommandHandler implements ActionListener {
View view;
public CommandHandler(View v) {
super();
view = v;
}
//TODO: this is ugly as hell
public void actionPerformed(ActionEvent e) {
JButton source = (JButton)e.getSource();
System.out.println(source.getText());
//if (source.getName().equals(CommandPanel.FIRE_BUTTON)) {
view.getObserver().update(view, source.getName()); //dummy
//}
}
}
}
CommandPanel.java (the only UI panel I have wired to the View currently)
public class CommandPanel extends JPanel {
/**
*
*/
private static final long serialVersionUID = -4323743110351491547L;
public static final int SHIP_SELECTED = 1;
public static final int NONE_SELECTED = 0;
public static final String FIRE_BUTTON = "fireButton";
public static final String MOVE_BUTTON = "moveButton";
public static final String TURN_BUTTON = "turnButton";
public static final String MINES_BUTTON = "minesButton";
private JButton fire, move, turn, mines;
public CommandPanel() {
super(new GridLayout(0, 1));
setPreferredSize(new Dimension(200, 200));
fire = new JButton("Fire");
fire.setName(FIRE_BUTTON);
add(fire);
move = new JButton("Move");
move.setName(MOVE_BUTTON);
add(move);
turn = new JButton("Turn");
turn.setName(TURN_BUTTON);
add(turn);
mines = new JButton("Mines");
mines.setName(MINES_BUTTON);
add(mines);
}
public void addHandler(ActionListener c) {
fire.addActionListener(c);
move.addActionListener(c);
turn.addActionListener(c);
mines.addActionListener(c);
}
}
Controller.java (completely barebones, I’m mostly working on how to nicely pass messages before I get any further into content)
public class Controller implements Observer {
private DataLoader loader;
private View view;
public Controller() {
//TODO: add a viewListener to this that listens to events from the view
//(provided by the view's subcomponents and translated to a generic
// event) and provide method execution based on events
loader = DataLoader.getInstance();
view = new View();
view.addObserver(this);
}
@Override
public void update(Observable o, Object arg) {
view.writeToLog(arg.toString());
}
}
E: after puzzling through a bunch of silly stuff I realized that I was going about this all wrong, and that my “View” class was actually my controller and I wasn’t understanding the paradigm nearly as well as I thought I was, which is making everything far easier.
Solution
A constructor should create an object that’s ready to use. It seems that an instance of CommandPanel would be utterly useless without also calling its addHandler method. And it seems you never call addHandler anywhere else, except right after creating a CommandPanel instance.
These signs point to that probably there shouldn’t be an addHandler method at all,
or that it should be private final,
and only called from the constructor.
Pass a CommandHandler into the constructor,
and either make addHandler method private final and call it from the constructor,
or move its content into the constructor and do all the necessary setup in there.
A benefit of moving the content of addHandler inside the constructor will be that all the JButton member variables can be converted to local variables.
And in CommandHandler, it would be good to make the view member variable private final.