6
\$\begingroup\$

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 BuildingFactory class really necessary? I mean: I could have done it with constructors, no?
\$\endgroup\$
  • 2
    \$\begingroup\$ You can find the entire code and further explanation on the GitHub repository at this commit. \$\endgroup\$ – Odepax Jun 26 '17 at 6:24
8
\$\begingroup\$

MVC

The MVC pattern is described like this: The controller knows the model and the view. The view knows the model. About how components communication with each other: I've seen different opinions on that, but none of them send messages with direct dependencies. Since the View has a dependency to the GameController: Nope, MVC is not correctly applied here.

You shouldn't only fixate on the question, if you have implemented the pattern correctly, but more on the question, if you have achieved what the pattern wants to achieve. It's about decoupling displaying of information, controlling of messages and flows, and the model and the handling of its states. Or in other words, make parts of your presentation layer interchangable, reusable and easier to expand.

I don't see enough of the code to give a proper or wide answer, but:

  • The View should be as stupid as possible
  • The game logic shouldn't be in the presentation layer itself, at least in my opinion. If you want to change your architecture on your presentation, it's much harder to do so. You also have some parts of it in the Controller, for instance the collision check of the buildings. Just think about this: If you want to make a console client for your game: What parts cannot be reused?
  • Get rid of Game.getInstance(), use dependency injection. Singleton is an anti pattern for several reasons and shouldn't be used for convencience.

GameView / BattlefieldView and ShopView aggregation

To couple components tightly should be avoided in general. In your case, the reusing of the same Listener seems like a good idea, but if you change the implementation, all components sharing the same Listener are affected. I also don't see a reason why those components should have the same Listener anyway.

Usually, it's a good idea to separate your view-model-controller couples from other couples, to keep the complexity down. "A class should have only one reason to change.". If you change the listener and have to change other classes, or the other way around, it's tightly coupled. Or: The concerns are not separated.

currentView instanceof BattlefieldView

That's a violation of liskov substitution principle. Or in other words: Bad inheritance. To cast it to a common View object would be okay, since the Listener only provides Object, but to instance-of the object to a specific type and downcast it should be avoided.

Swing

You mentioned, you want to keep updating the game, so I - and also Oracle - recommend to use JavaFX instead of Swing.

Thread safety

I wouldn't go and synchronize everything. Why should a getter be synchronized, if nothing is updating? It might be okay for your game, but not for an application with heavy load. If I were you, I'd only synchronize if necessary.

The Collections.synchronizedList: Check the java doc: Iterations should be synchronized.

BuildingFactory

Well, haven't seen a code from that one. But one good reason for a static factory method is: You can give the method which creates the Building a proper name, which is not possible for constructors. If you see a class with two constructors, you usually have no idea which one to call, a method name helps a lot.

Testing

I did not see a single unit test. Since you mentioned you are in university, and my experience is, that people do not learn how to write proper unit tests and do not learn how to grow an application with unit tests during their education - at least I did not, nor did my apprentices -: Please write unit tests. Not only will it help you verify and document your application, but it will also help you with your design. There are a few places in your code where testing will be hard or even: not possible. And this is always the sign of bad design. And: In the real world, testing is a crucial part of software development.

Other / smaller stuff

  • Isn't there a drag and drop mechanism in swing? Instead of using mouseClicked/released and remembering the information...
  • The Game model facade: Why is the Game class a facade?
  • AttackTroop troop = source.detachTroopTo(target);. Not sure about this one: But does a building send troops to another building? Is it really the buildings task?
  • addAttackTroop(troop): No idea what that does? Add attack troop to where? Why?
  • setChanged();: Neither do I understand this one, I assume it's the trigger for the observers - a proper name usually helps.
\$\endgroup\$
  • \$\begingroup\$ MVC --- 1. Do you suggest to move "the collision check" to the View layer? --- 2. "The controller knows the view": Do you have a link to a good example? All of those I've ever seen injected the controller in the view. Also, I must be influenced by MVVM. --- 3. "Why those components should have the same Listener": Because you Drag & Drop from and to different panels. I'm not sure that I can get the coordinates of the drop point to apply the collision; can I? \$\endgroup\$ – Odepax Jun 28 '17 at 9:00
  • \$\begingroup\$ Thread Safety --- "Collections.synchronizedList": Does this remark apply to Vector<T> too? \$\endgroup\$ – Odepax Jun 28 '17 at 9:01
  • \$\begingroup\$ Other Stuff --- 1. "Why is Game a facade": It's the only model object which is manipulated by the other layers, the view and the controller never change the properties of the entities directly. --- 2. detachTroopTo() returns an AttackTroop that is initialized with the right target. --- 3. addAttackTroop() adds the troop to the moving troops list, it's used by the game loop thread to animate their movement. --- 4. setChanged() comes from Observable. \$\endgroup\$ – Odepax Jun 28 '17 at 9:01
  • \$\begingroup\$ MVC 1: Nope, to the game logic (think about console client). View must be as stupid as possible. 2: See Martin Fowler's website, he has lots of good articles about this kind of stuff. 3. As far as I remember: There should be a drag and drop mechanism in Swing. Thread safety: IMO Vector is deprecated. If you want to know: Check the sources. \$\endgroup\$ – slowy Jun 28 '17 at 15:25
  • \$\begingroup\$ Other stuff: 1. I have a different understanding about the facade design pattern ;-). 2. Well, I don't think a building should have the functionality to send a troop to another builder (separation of concerns). 3. Hm, okay. I'd given then troop a state "attacking", which will be rendered differently by the renderer (just a hint). The Game does work for the renderer, doesn't it? That's a violation of SRP, too, imo. 4. That's what I thought. The problem is, I wasn't sure. If you can rename the method, so another reader can be (more) sure, rename it (notifyObservers() maybe?) \$\endgroup\$ – slowy Jun 28 '17 at 15:36

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.