I developed a mini game in Java Swing for the University; I'd like to carry it on as a personal project. But before that, I'd like to know if what I've done so far is correct or not.
The game itself is a Java implementation of the Civilization Wars Flash game: there is a rectangular map with buildings on it, the player has to Drag & Drop from a building to another in order to detach some troops and make them capture the enemy building. In addition, I've added a shop, in which the player can get modules. A module applies to a building to buff it.
Main Concerns
I'll try to include here the most relevant parts of the code for the review.
Architecture
I'll take the example of the BattlefieldView class, but all the views are organized the same way: an event handler catches the event and passes a request to the controller, the controller receives the request and transforms the data of the event into business data understandable by the model facade.
view.BattlefieldView.MouseAdapter#mouseReleased(MouseEvent) // The event handler
controller.GameController#sendTroops(Point2D, Point2D) // calls the controller's method
model.Game#sendTroops(model.Building, model.Building) // which transforms the data and calls the facade
model.Buildin#detachTroopTo(model.Building) // which modifies the model.
I've got rid of some useless stuff like the paintComponent method to make the code more concise.
class BattlefieldView extends JPanel {
BattlefieldView(GameController gameController) {
addMouseListener(new MouseAdapter() {
private Point2D source;
public void mousePressed(MouseEvent e) {
if (source == null) {
source = new Point2D.Double(e.getX(), e.getY());
}
}
public void mouseReleased(MouseEvent e) {
if (source != null) {
Point2D target = new Point2D.Double(e.getX(), e.getY());
gameController.sendTroops(source, target);
}
source = null;
}
});
Game.getInstance().addObserver((source, arg) -> { repaint(); });
}
}
The GameController#sendTroops's method just converts the received coordinates into model objects:
class GameController {
// TODO: Should I move this method to the civwar.model.Game class?
private Building getBuildingByCollision(Point2D point) {
for (Building building : Game.getInstance().getBuildings()) {
if (Math.abs(building.getPosition().getX() - point.getX()) <= 10 &&
Math.abs(building.getPosition().getY() - point.getY()) <= 10
) {
return building;
}
}
return null;
}
public void sendTroops(Point2D source, Point2D target) {
Building sourceBuilding = getBuildingByCollision(source);
if (sourceBuilding == null) return;
Building targetBuilding = getBuildingByCollision(target);
if (targetBuilding == null) return;
Game.getInstance().sendTroops(sourceBuilding, targetBuilding);
}
}
The Game model facade and its sendTroops method:
class Game extends Observable {
public void sendTroops(Building source, Building target) {
AttackTroop troop = source.detachTroopTo(target);
if (troop != null) {
addAttackTroop(troop);
setChanged();
}
}
}
Is MVC correctly applied here? Especially when it comes to the controller layer (Event handlers call the Controller, which calls the Model)? I have the impression that my Controller layer, i.e. the only class of the controller package, is here just to allow me to say "Hey look at me, I've a control layer!", that the event handlers could call the model directly. Should I have done it differently or is that "the right way" as it is?
Also, the BattlefieldView is a component of the GameView, which also includes the ShopView:
class GameView extends JPanel {
public GameView(GameController gameController) {
MouseListener moduleApplicationListener = new MouseAdapter() {
private Module module;
private Object currentView;
public void mouseEntered(MouseEvent e) { currentView = e.getSource(); }
public void mousePressed(MouseEvent e) {
if (currentView instanceof ModuleView && module == null) {
module = ((ModuleView)currentView).getModule();
}
}
public void mouseReleased(MouseEvent e) {
if (currentView instanceof BattlefieldView && module != null) {
Point battlefieldViewLocation = ((BattlefieldView)currentView).getLocationOnScreen();
gameController.applyModule(module, new Point2D.Double(
e.getXOnScreen() - battlefieldViewLocation.getX(),
e.getYOnScreen() - battlefieldViewLocation.getY()
));
}
module = null;
}
};
BattlefieldView battlefieldView = new BattlefieldView(gameController);
battlefieldView.addMouseListener(moduleApplicationListener);
ShopView shopView = new ShopView(moduleApplicationListener);
}
}
The moduleApplicationListener MouseListener object is passed in the constructor of the ShopView, which propagates it to its children: the ModuleView objects.
class ShopView extends JPanel {
ShopView(MouseListener moduleApplicationListener) {
for (Module module : Game.getInstance().getModules()) {
ModuleView moduleButton = new ModuleView(module);
moduleButton.addMouseListener(moduleApplicationListener); // Add the listener to the button.
add(moduleButton);
}
}
}
This way, the BattlefieldView and the ModuleViews share a common event listener. This allows the user to Drag & Drop modules from the shop. I'd like to know what you think about this way of sharing the event handler.
Thread Safety
Thread safety is implemented in the model entities: all getters, setters and operations are synchronized. On top of that, they use a built-in thread-safe implementation of List<T>.
Is what I've done too much? Not enough?
Secondary Concerns
- Is it a good choice to put the
GameLoop(timer thread) in the Model? - Is the
BuildingFactoryclass really necessary? I mean: I could have done it with constructors, no?
