5
\$\begingroup\$

I have created a UI shown in the screenshot. I am initalizing too many JPanels within a panel in my code to achieve the UI which will lead to bad code design and slower performance.

Is there any good practices to follow in writing such a UI?

enter image description here

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.GridLayout;
import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;

import javax.imageio.ImageIO;
import javax.swing.BorderFactory;
import javax.swing.Box;
import javax.swing.BoxLayout;
import javax.swing.ImageIcon;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextArea;
import javax.swing.JTextField;
import javax.swing.SwingConstants;


public class GameUI extends JFrame {

    static JLabel blankCardLabel;
    static JLabel cardOne;
    JLabel cardTwo;
    JLabel cardThree;
    JLabel cardFour;
    JLabel cardFive;
    JLabel cardSix;
    JLabel cardSeven;
    JLabel cardEight;
    JLabel cardNine;
    JLabel cardTen;
    JLabel cardEleven;
    JLabel cardTwelve;
    JLabel cardThirteen;
    JLabel cardFourteen;
     BufferedImage blankCardPic;
    JLabel deck;
     BufferedImage revealCardPic;
    JPanel eastPanel;
    JPanel southPanel;
    JPanel messagePanel;
    JPanel cardPanel;
    BoxLayout eastPanelBL;
    BoxLayout southPanelBL;
    BoxLayout cardPanelBL;
    JLabel playerOne = new JLabel("Player One");
    JLabel playerTwo = new JLabel("Player Two");
    JLabel playerThree = new JLabel("Player Three");
    JLabel playerFour = new JLabel("Player Four");
    JLabel c;
    public GameUI() {
        setLayout(new BorderLayout());

        add(new JLabel("Memory Game", SwingConstants.CENTER), BorderLayout.NORTH);
        /*cardOne = new JLabel("One");
        cardTwo = new JLabel("Two");
        cardThree = new JLabel("Three");
        cardFour = new JLabel("Four");
        cardFive = new JLabel("Five");
        cardSix = new JLabel("Six");
        cardSeven = new JLabel("Seven");
        cardEight = new JLabel("Eight");
        cardNine = new JLabel("Nine");
        cardTen = new JLabel("Ten");
        cardEleven = new JLabel("Eleven");
        cardTwelve = new JLabel("Twelve");
        cardThirteen = new JLabel("Thirteen");
        cardFourteen = new JLabel("Fourteen");*/

        deck = new JLabel(new ImageIcon(blankCard()));
        cardOne = new JLabel(new ImageIcon(blankCard()));
        cardTwo = new JLabel(new ImageIcon(blankCard()));
        cardThree = new JLabel(new ImageIcon(blankCard()));
        cardFour = new JLabel(new ImageIcon(blankCard()));
        cardFive = new JLabel(new ImageIcon(blankCard()));
        cardSix = new JLabel(new ImageIcon(blankCard()));
        cardSeven = new JLabel(new ImageIcon(blankCard()));
        cardEight = new JLabel(new ImageIcon(blankCard()));
        cardNine = new JLabel(new ImageIcon(blankCard()));
        cardTen = new JLabel(new ImageIcon(blankCard()));
        cardEleven = new JLabel(new ImageIcon(blankCard()));
        cardTwelve = new JLabel(new ImageIcon(blankCard()));
        cardThirteen = new JLabel(new ImageIcon(blankCard()));
        cardFourteen = new JLabel(new ImageIcon(blankCard()));


        cardPanel = new JPanel();
        cardPanelBL = new BoxLayout(cardPanel, BoxLayout.Y_AXIS);
        cardPanel.setLayout(cardPanelBL);

        JPanel topRowPanel = new JPanel();

        //cardPanel.add(deck);
        //c= new JLabel("Card Left In deck",SwingConstants.CENTER);
        //cardPanel.add(c);

        topRowPanel.add(cardOne);
        topRowPanel.add(cardTwo);
        topRowPanel.add(cardThree);
        topRowPanel.add(cardFour);
        topRowPanel.add(cardFive);
        topRowPanel.add(cardSix);
        topRowPanel.add(cardSeven);
        cardPanel.add(topRowPanel);

        JPanel bottomRowPanel = new JPanel();
        bottomRowPanel.add(cardEight);
        bottomRowPanel.add(cardNine);
        bottomRowPanel.add(cardTen);
        bottomRowPanel.add(cardEleven);
        bottomRowPanel.add(cardTwelve);
        bottomRowPanel.add(cardThirteen);
        bottomRowPanel.add(cardFourteen);
        cardPanel.add(bottomRowPanel);


        add(cardPanel,BorderLayout.CENTER);
        //cardOne.setBounds(50,50,50,50);
        eastPanel = new JPanel();
        eastPanelBL = new BoxLayout(eastPanel, BoxLayout.Y_AXIS);
        eastPanel.setLayout(eastPanelBL);
        eastPanel.add(Box.createRigidArea(new Dimension(0, 20)));
        eastPanel.add(playerOne);
        eastPanel.add(Box.createRigidArea(new Dimension(0, 5)));
        eastPanel.add(playerTwo);
        eastPanel.add(Box.createRigidArea(new Dimension(0, 5)));
        eastPanel.add(playerThree);
        eastPanel.add(Box.createRigidArea(new Dimension(0, 5)));
        eastPanel.add(playerFour);
        add(eastPanel, BorderLayout.EAST);  
        southPanel = new JPanel(new BorderLayout());
        southPanel.add(new JTextArea(10,10),BorderLayout.CENTER);
        messagePanel = new JPanel(new BorderLayout());
        messagePanel.add(new JTextField(10),BorderLayout.CENTER);
        messagePanel.add(new JButton("Send"),BorderLayout.EAST);
        southPanel.add(messagePanel,BorderLayout.SOUTH);
        add(southPanel, BorderLayout.SOUTH);        

        //add(cardOne);
        //cardOne.setText("ClubsAce.png");

        setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        pack();
        setSize(1024,768);
        setLocationRelativeTo(null);
        setResizable(false);
        setVisible(true);
    }

    public static void main(String[] args) {
        new GameUI();
    }

    public BufferedImage blankCard() {
        try {
            blankCardPic = ImageIO.read(new File("resources/images/blank.png"));
        } catch (IOException e) {
            // TODO Auto-generated catch block
            e.printStackTrace();
        }
        return blankCardPic;
    }

    /*public static BufferedImage revealCard(String s) {
        try {
            revealCardPic = ImageIO.read(new File("resources/images/"+s));
        } catch (IOException e) {
            e.printStackTrace();
        }
        return revealCardPic;
    }*/
}
\$\endgroup\$
1
  • \$\begingroup\$ Congratulation on your game! Help me understand why did you choose that technology to deliver your game? \$\endgroup\$ – Deian Jul 21 '16 at 18:37
3
\$\begingroup\$

Goal

Alright, before diving into the code, let's take a look at the screenshot you attached and break down what you're trying to accomplish. I see the following UI components:

  1. A game title area
  2. A panel to keep track of whose turn it is
  3. A chat area
  4. The main 'game space'

From the looks of it, you've already accomplished this to some extend. You've created JPanels for each main section:

  • JPanel eastPanel
  • JPanel southPanel
  • JPanel messagePanel
  • JPanel cardPanel

The naming can be improved, but good start breaking things up!

What we want to do is break up this massive GameUI class into components that are modular, and have a single responsibility (Single Responsibility Principle). Also, I'm partial to the MVP (Model-View-Presenter) design pattern, and will be following that as we go. This isn't the only correct way to build a UI application, but I find it to be pretty intuitive once you're used to it, and is a great way to see the Single Responsibility Principle in action.

Another added benefit of splitting everything up is that we avoid a massive block of unrelated variable declarations at the top of a class :)


Design

I just mentioned the MVP pattern, so what we'll do here is outline what that looks like for each UI component (from above) and what classes will be needed.

Game Title

Frankly, this should really just be the title of the JFrame:

final JFrame frame = new JFrame("Memory Game");

Now we're left with just 3 UI components.

User Panel

The basis of MVP is that you have 3 types of classes: models, views, presenters.

  1. The view is obviously what you see. The view should be very dumb, and not contain business logic (any logic that determines what should be shown if some event occurs)
  2. The presenter will listen listen for events from the view, then do the business logic, then turn around and then update the view.
  3. The models are mostly POJOs that keep the state of the application. The models are updated via the presenter.

That's a very over-simplified overview, so I recommend reading some other sources and looking at other examples.

With that in mind, let's look at this user panel. We will want a class to represent the UI aspect (the V in MVP):

public class UserPanelView {
    public UserPanelView() {
        // Setup all the UI elements
    }
}

We also want a presenter:

public class UserPanelPresenter {
    public UserPanelPresenter() {
        // ...
    }
}

Chat Area

Just like the User Panel, we want a view and a presenter:

public class ChatPanelView {
    public ChatPanelView() {
        // Setup all the UI elements
    }
}

public class ChatPanelPresenter {
    public ChatPanelPresenter() {
        // ...
    }
}

Game Area

Again, we want a view and a presenter:

public class GameBoardView {
    public GameBoardView() {
        // Setup all the UI elements
    }
}

public class GameBoardPresenter {
    public GameBoardPresenter() {
        // ...
    }
}

This is all fine and great, but now we're left with 6 classes that don't have any connection to each other! To remedy this, we can realize that we have a fourth (or fifth if you want to count the game title still) UI component: time entirety of the application window!

Game Window

As before:

public class MemoryGameView {
    public MemoryGameView() {
        // Setup all the UI elements
    }
}

public class MemoryGamePresenter {
    public MemoryGamePresenter() {
        // ...
    }
}

I'm also going to introduce one last class which will have our main method:

public class MemoryGameApplication {
    public static void main(String... args) {
        // ...
    }
}

The classes are still disjoint, but we're getting closer - now we have a main class to begin to tie everything together!

Bringing the classes together

Recall that earlier I had mentioned that a presenter needs to listen to the view. To accomplish this, we create a listener interface for each view which the presenter will implement. We will then inject the view into the presenter, so that the presenter can add itself as a listener to the view. That's a lot of confusing words, so put concretely, we can do the following with the Chat Area, for example:

public interface ChatPanelViewListener {
    /**
     * Method to be called when the Send button is clicked.
     */
    public void onSendButtonClicked();
}

public class ChatPanelPresenter implements ChatPanelViewListener {
    private final ChatPanelView view;

    public ChatPanelPresenter(final ChatPanelView view) {
        this.view = view;
        view.addListener(this);
    }

    @Override
    public void onSendButtonClicked() {
        // ... the business logic ...
        // ... call some method(s) on the view to update it ...
    }
}

public class ChatPanelView {
    private final List<ChatPanelViewListener> listeners;

    public ChatPanelView() {
        listeners = new ArrayList<ChatPanelViewListener>();
        // ...
        final JButton sendButton = new JButton("Send");
        sendButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(final ActionEvent e) {
                notifyListenersOnSendButtonClicked();
            }
        });
        // ...
    }

    public void addListener(final ChatPanelViewListener listener) {
        listeners.add(listener);
    }

    private void notifyListenersOnSendButtonClicked() {
        for (final ChatPanelViewListener listener : listeners) {
            listener.onSendButtonClicked();
        }
    }
}

Ok, so in summary what we just did was wire up the view and the presenter in such a way that the view will notify the presenter when the button is clicked. Then, the presenter can do some logic, or whatever it needs to do (It's a chat area, so maybe send a message over the network eventually??), then turn around and call some yet-to-be-written methods on the view to update it (Remember: dumb methods only! ie. setText(), etc. (no complex logic!)). Again, this isn't the only way to do it, but I've found it to be a great way!

This same process can be applied to other components as well! It doesn't look like your user panel is going to be responding to any user input (instead, the presenter will just be updating the view), so we can probably forgo creating a UserPanelViewListener for now:

public class UserPanelPresenter {
    private final UserPanelView view;

    public UserPanelPresenter(final UserPanelView view) {
        this.view = view;
    }
}

Before we tackle the game board, let's see how the User Panel and the Chat Panel can be setup from the start:

public class MemoryGameView {
    private final UserPanelView userPanelView;
    private final ChatPanelView chatPanelView;

    public MemoryGameView() {
        // Allow the view to create the UI elements in the correct order and layout
        userPanelView = new UserPanelView();
        chatPanelView = new ChatPanelView();
    }

    // Expose these methods to allow the views to be wired into their respective presenters
    public UserPanelView getUserPanelView() {
        return userPanelView;
    }
    public ChatPanelView getChatPanelView() {
        return chatPanelView;
    }
}

public class MemoryGamePresenter {
    private final MemoryGameView view;
    private final UserPanelPresenter userPanelPresenter;
    private final ChatPanelPresenter chatPanelPresenter;

    // Inject the components into the presenter - This makes testing MUCH easier because you can mock these dependencies!
    public MemoryGamePresenter(final MemoryGameView view, final UserPanelPresenter userPanelPresenter, final ChatPanelPresenter chatPanelPresenter) {
        this.view = view;
        this.userPanelPresenter = userPanelPresenter;
        this.chatPanelPresenter = chatPanelPresenter;
    }
}

public class MemoryGameApplication {
    public MemoryGameApplication() {
        final MemoryGameView view = new MemoryGameView();
        final UserPanelPresenter userPanelPresenter = view.getUserPanelPresenter();
        final ChatPanelPresenter chatPanelPresenter = view.getChatPanelPresenter();
        new MemoryGamePresenter(view, userPanelPresenter, chatPanelPresenter);
    }

    public static void main(String... args) {
        new MemoryGameApplication();
    }
}

What we've just done is allowed the MemoryGameView to create a heirarchy of UI elements. Then, we wire up the views and their presenters. Now everything is connected! You setup the game in a very similar way via your main method - great job! It's easy to fall into the trap of loading up the main method with lots of extra work that doesn't belong there, and should be pulled out. Adding the GameBoadView, GameBoardPresenter and GameBoardViewListener is straightforward, and follows the exact same pattern as the other classes.

Displaying the UI & Making It Look Good

In your code, your GameUI class extends JFrame. This isn't necessarily wrong, however I prefer composition over inheritance (See: Prefer composition over inheritance and How to decide whether use IS A or HAS A Relation. Furthermore, I would prefer that each view class were instead an interface which defines all the methods that the view needs. Then, you would create an implementation that is specific to the UI framework you are using. I won't go into it any further, but this would allow you to create a version of the application using a different UI framework by only changing the view classes!

Starting from a high level we want to create the UI to be a 1024x768 window, and have the title be "Memory Game". To do so, we can pass these parameters to the MemoryGameView constructor.

/**
 * Constructor
 * @param width The width of the game window. Must be a positive number.
 * @param height The height of the game window. Must be a positive number.
 * @param title The title to be display on the game window. Cannot be null.
 */
public MemoryGameView(final int width, final int height, final String title) {
    // Validate the parameters and do the stuff
}

We can also setup the basis of the UI in this constructor because it is the top-level view! These base panels correspond to the east, south and card panels you have in your code - I just changed the names to make a bit more sense. The book Clean Code is a wonderful resource that has lots of great suggestions for naming variables, classes and the like. Here's some snippets about naming: Clean Code: Chapter 2, “Meaningful Names”

public MemoryGameView(final int width, final int height, final String title) {
    final JFrame frame = new JFrame(title);
    final Container pane = frame.getContentPane();

    // Formerly the cardsPanel
    final JPanel gameBoardPanel = new JPanel();
    pane.add(gameBoardPanel, BorderLayout.CENTER);

    // Formerly the eastPanel
    final JPanel userPanel = new JPanel();
    pane.add(userPanel, BorderLayout.EAST);

    // Formerly the southPanel
    final JPanel chatPanel = new JPanel();
    pane.add(chatPanel, BorderLayout.SOUTH);

    // ... create UserPanelView and ChatPanelView ...

    // ... and copying over what you've already written ...
    frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
    frame.pack();
    frame.setSize(width, height);
    frame.setLocationRelativeTo(null);
    frame.setResizable(false);
    frame.setVisible(true);
}

What we've done is loosely arrange the base panels for each subview. Those panels can then be passed into the constructors of the subviews so that the view can create its own elements on that parent panel.

For example, using more or less the same code you already have (woohoo!), just cleaning up some of the naming, we get:

public ChatPanelView(final JPanel parent) {
    final JPanel basePanel = new JPanel(new BorderLayout());

    final JTextArea messagesTextArea = new JTextArea(10, 10);
    basePanel.add(messagesTextArea, BorderLayout.CENTER);

    final JPanel inputsPanel = new JPanel(new BorderLayout());
    basePanel.add(inputsPanel, BorderLayout.SOUTH);

    final JTextField inputField = new JTextField(10);
    inputsPanel.add(inputField, BorderLayout.CENTER);

    final JButton sendButton = new JButton("Send");
    inputsPanel.add(sendButton, BorderLayout.EAST);

    parent.setLayout(new BorderLayout());
    parent.add(basePanel);

    // ... other stuff ...
}

Similarly, for the UserPanelView:

public class UserPanelView {
    private final JPanel usersPanel;

    public UserPanelView(final JPanel parent) {
        final JPanel basePanel = new JPanel(new BorderLayout());
        // Instead of having a large margin on the first username to slide things down, we can have a border around the panel to create a margin around all of the usernames
        basePanel.setBorder(new EmptyBorder(20, 20, 20, 20));
        basePanel.setPreferredSize(new Dimension(200, 100));

        usersPanel = new JPanel();
        final BoxLayout usersPanelLayout = new BoxLayout(usersPanel, BoxLayout.Y_AXIS);
        usersPanel.setLayout(usersPanelLayout);
        basePanel.add(usersPanel, BorderLayout.CENTER);

        parent.setLayout(new BorderLayout());
        parent.add(basePanel, BorderLayout.CENTER);
    }

    public void addUser(final String username) {
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                final JLabel userLabel = new JLabel(username);
                usersPanel.add(userLabel);
                usersPanel.revalidate();
                usersPanel.repaint();
            }
        });
    }
}

Notice that here we did a couple of things slightly different. For one, we didn't add all of the users in the constructor. What if we want to add more later? As it was, we would have been out of luck! Instead, we delegate that responsibility to the presenter who can call addUser(...) whenever necessary.

Second, we have an odd bit of code going on with the SwingUtilities.invokeLater(...). In Swing, it is up to you to update UI code on a UI thread. Some other frameworks will do this for you (I believe AWT is one of them), but not in Swing. I don't use Swing very often, so I will defer explaining this and point you to a few links instead: - SwingUtilities.invokeLater() - Why is it neede? - invokeLater() - What is the advantage of the SwingUtilities.invokeLater() method? et. al.

In fact, we really should have wrapped the construction of our application in a call to invokeLater() as well:

public static void main(String... args) {
    SwingUtilities.invokeLater(new Runnable() {
        @Override
        public void run() {
            new MemoryGameApplication();
        }
    });
}

Third, instead forcing the top username to have extra space at the top, I created a usersPanel under the basePanel which can have a border and thus some padding. I must admit that I don't use Swing often at all, and setting concrete dimensions like I did with basePanel.setPreferredSize() is really not a good idea, but only the width is respected here, and the panel will still take up the full height that is available.

Great! Now we can move on to the cards!

We can start off just like the other views:

public GameBoardView(final JPanel parent) {
    final JPanel basePanel = new JPanel();

    cardsPanel = new JPanel();
    cardsPanel.setLayout(new GridLayout());
    cardsPanel.setBackground(Color.WHITE);
    basePanel.add(cardsPanel, BorderLayout.CENTER);

    parent.setLayout(new GridBagLayout());
    final GridBagConstraints gridBagConstraints = new GridBagConstraints();
    gridBagConstraints.fill = GridBagConstraints.BOTH;
    parent.add(basePanel, gridBagConstraints);
}

...except this time we can use a GridBagLayout to center everything.

Getting the Resources

Let's take a minute to look at how you're loading the images for the cards that you want to display.

From where?

+1 for placing your images in a resource directory! If you're not familiar with Maven for managing dependencies, I would highly recommend learning about it, but it expects the following source folder structure: - src/main/java - src/main/resources - src/test/java Even if you don't use Maven, it's a good habit to organize appropriately.

How?

The way you're reading a single image now is a perfectly valid way to do so. However, each time you call blankCard(), you're reading the same file over and over again, and assigning to the same static variable over and over again which is very inefficient. Instead, you can read the file once, and use the same ImageIcon object once for each label.

One alternative to creating the intermediate BufferedImage is to just create the ImageIcon directly. I'll address the filename shortly, but we will not be hard-coding the file path for each image into the code - that doesn't belong in the code! Also I found out about getScaledInstance() which I think is pretty handy if you don't want to bother with resizing your resource images by hand. I'm lazy :)

new ImageIcon(new ImageIcon(filename).getImage().getScaledInstance(cardWidth, cardHeight, Image.SCALE_SMOOTH));

Looking at the block of code creating all the JLabel objects it's pretty clear that there is a LOT of duplicate work going on here. I'm going to propose an approach in this next section that may seem a bit over-engineered, but bear with me - I think it will help illustrate ways to break code into pieces that have a single responsibility.

To where?

I'm going to suggest creating a theme class to hold all of your images. I see several advantages to this:

  1. What if two views need the same image? A theme class can eliminate duplicates.
  2. Not applicable for an ImageIcon, but how can we cleanly dispose of the image resources? (eg. Image.dispose(). A theme class can have a single dispose() method to dispose all of the resources at once when we exit the application.
  3. A single instance of the theme can be handed down to views instead of a series of individual resources.
  4. All resource loading can be done at one time upfront.

With that in mind, this is one way it can be done:

public class MemoryGameTheme {
    private final Map<String, ImageIcon> cardFaceIcons;
    private final ImageIcon cardBackIcon;

    public MemoryGameTheme(final List<Card> cards, final int cardWidth, final int cardHeight) {
        cardFaceIcons = new HashMap<String, ImageIcon>();
        for (Card card : cards) {
            cardFaceIcons.put(card.name(), getScaledCardImage(card.name(), cardWidth, cardHeight));
        }
        cardBackIcon = getScaledCardImage("BACK", cardWidth, cardHeight);
    }

    private ImageIcon getScaledCardImage(final String resourceKey, final int cardWidth, final int cardHeight) {
        final String filename = ResourceBundle.getBundle("IconFilenameBundle").getString(resourceKey);
        return new ImageIcon(new ImageIcon(filename).getImage().getScaledInstance(cardWidth, cardHeight, Image.SCALE_SMOOTH));
    }

    public ImageIcon getCardFaceIconForName(final String cardName) {
        return cardFaceIcons.get(cardName);
    }

    public ImageIcon getCardBackIcon() {
        return cardBackIcon;
    }
}

I made a Card enum which I'll get to later, but all it does is let you define the different cards you want in your game:

public enum Card {
    TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, TEN, JACK, QUEEN, KING, ACE;
}

The purpose of a ResourceBundle is to handle locale-specific objects - primarily any String that might need to be displayed in multiple languages. But we can also use it to load up the file paths for our images! By using ResourceBundle.getBundle("IconFilenameBundle") we can load file path strings from a IconFilenameBundle.properties file located in src/main/resources. That file will just contain a bunch of key/value pairs for the card name and image location. For example:

BACK = src/main/resources/images/back.jpg
TWO = src/main/resources/images/2.png
...
ACE = src/main/resources/images/ace.png

The advantage to this is that we don't have to update any code if we want to point to a different image.

Back in GameBoardView we can pass in an instance of our theme, and instead of calling your blankCard() method and creating new instances of the same image over and over, we can now call the getCardBackIcon() method.

public class GameBoardView {

    private final Map<String, JLabel> cardLabels; // For easy access later when you need to show the other side of the card!

    public GameBoardView(final JPanel parent, final MemoryGameTheme theme) {
        // ...
        cardLabels = new HashMap<String, JLabel>();
    }

    public void setCards(final List<String> cardNames) {
        cardsPanel.removeAll(); // Remove existing cards if there were any
        cardLabels.clear();
        cardsPanel.setLayout(new GridLayout(2, (cardNames.size() + 1) / 2, 20, 20));

        for (final String cardName : cardNames) {
            final JLabel cardLabel = new JLabel(theme.getCardBackIcon());
            cardLabel.setName(cardName);
            cardLabel.addMouseListener(new MouseAdapter() {
                @Override
                public void mouseClicked(MouseEvent e) {
                    notifyListenersOnCardSelected(cardLabel.getName());
                }
            });
            cardsPanel.add(cardLabel);
            cardLabels.put(cardName, cardLabel);
        }

        cardsPanel.revalidate();
        cardsPanel.repaint();
    }

    private void notifyListenersOnCardSelected(final String cardName) {
        for (final GameBoardViewListener listener : listeners) {
            listener.onCardSelected(cardName);
        }
    }

}

public class GameBoardPresenter implements GameBoardViewListener {

    public GameBoardPresenter(final GameBoardView view) {
        // ...
        // In reality, you can make a way to get a random set of pairs of cards, but I'll leave that up to you :)
        final List<String> cards = new ArrayList<String>();
        cards.add(Card.ACE.name());
        cards.add(Card.ACE.name());
        cards.add(Card.FOUR.name());
        cards.add(Card.FOUR.name());
        cards.add(Card.QUEEN.name());
        cards.add(Card.QUEEN.name());
        view.setCards(cards);
    }

    @Override
    public void onCardSelected(final String cardName) {
        final Card card = Card.valueOf(cardName);
        // Do the logic for flipping the card, checking for a match, etc.
        // I'll leave this up to you, as you haven't provided any code for this yet :)
    }

}

I know that seems like a lot, but we're keeping the view dumb, remember? We tell the view to show some cards, and then when one is selected, we can notify the presenter. Then the presenter can dictate how to proceed (flip over the card, check for a match, etc.

Creating setCards() also allows you to keep playing the game, just by resetting the cards at the end of a match - something you couldn't do as easily by declaring all of the labels upfront in a single class.

To flip the cards around, we can add methods to the view:

public void showCardFace(final String cardName) {
    cardLabels.get(cardName).setIcon(theme.getCardFaceIconForName(cardName));
}

public void showCardBack(final String cardName) {
    cardLabels.get(cardName).setIcon(theme.getCardBackIcon());
}

Remember, let the presenter handle the logic for when to show the face or the back! The view will simply do as the presenter commands.

Miscellaneous

  1. Variable Naming

    • eastPanelBL = new BoxLayout(...)
      • If I'm skimming through your code and see eastPanelBL, I won't have any idea what BL is. Also, it would be better to name the variable after what it's for or what it does. For example, your eastPanel is for the players/users area, so how about usersPanel or playersPanel instead? As I showed earlier if I have nested panels I will use basePanel, then something more specific for what each sub panel does.
    • Similarly, topRowPanel, bottomRowPanel: top and bottom rows of what?
  2. Make things final whenever you can! This will help prevent some easy-to-prevent mistakes. Almost all of your JLabel objects are not final, yet they're only set in the constructor. You'll want to break this class apart (either in a way like I've shown above, or some other way), but regardless they could have all been final.

  3. Document your code! Minimally you should have Javadoc on all public methods and classes. IDEs make it very easy to generate the templates for Javadoc on a method and list out the different parameters.
  4. Magic Numbers - don't use them. For example, in your code you have Box.createRigidArea(0, 5) and Box.createRigidArea(0, 20). Why 5 and sometimes 20? A combination of using a better hierarchy of UI components and some variables can remedy this (eg. usernameLabelMargin, etc.)
  5. Error handling. In your blankCard() method you're eating the IOException if it gets thrown and returning (probably) null. When handling exceptions you need to ask yourself whether or not it's something that you can recover from. In this case, you're potentially missing a resource, and your program will not function without that, so catching the exception, printing to the console, and carrying on your merry way doesn't make much sense. This ties back to my suggestion for a theme class which loads the resources at the start, and will throw an exception if a resource is not found.
  6. Not a comment on your code, but I mentioned "mocking" in one of my code comments. Take a look at the Mockito website and some examples to get an idea of why this is important.

Summary

Hopefully this doesn't come across like a top-to-bottom rewrite of your code. My intent is to demonstrate a way to take the single, massive class and break it into modular components. In doing so, you can literally cut and paste blocks of your code into the newly created classes. For example, your JPanel hierarchy works great with a few tweaks to accomodate new classes, and setting up the JFrame is correct. You also correctly added the JButton and listener to respond to input.

Below I'll put the complete code that I ended up with after making the changes mentioned above. Hopefully it can be of some benefit to see everything pieced together this way:


MemoryGameApplication

/**
 * Base class for the Memory Game application.
 * http://codereview.stackexchange.com/questions/129522/ui-based-memory-game
 */
public class MemoryGameApplication {

    /**
     * Constructor
     */
    public MemoryGameApplication() {
        final MemoryGameTheme theme = new MemoryGameTheme(Arrays.asList(Card.values()), 100, 150);
        final MemoryGameView view = new MemoryGameView(1024, 768, "Memory Game", theme);
        final UserPanelPresenter userPanelPresenter = new UserPanelPresenter(view.getUserPanelView());
        final ChatPanelPresenter chatPanelPresenter = new ChatPanelPresenter(view.getChatPanelView());
        final GameBoardPresenter gameBoardPresenter = new GameBoardPresenter(view.getGameBoardView());
        new MemoryGamePresenter(view, userPanelPresenter, chatPanelPresenter, gameBoardPresenter);
    }

    @SuppressWarnings("javadoc")
    public static void main(String... args) {
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                new MemoryGameApplication();
            }
        });
    }

}

MemoryGamePresenter

/**
 * Responsible for handling user interaction and updating the
 * {@link MemoryGameView}.
 */
public class MemoryGamePresenter {

    private final MemoryGameView view;
    private final UserPanelPresenter userPanelPresenter;
    private final ChatPanelPresenter chatPanelPresenter;
    private final GameBoardPresenter gameBoardPresenter;

    /**
     * Constructor
     * @param view The {@link MemoryGameView}. Cannot be null.
     * @param userPanelPresenter The {@link UserPanelPresenter}. Cannot be null.
     * @param chatPanelPresenter The {@link ChatPanelPresenter}. Cannot be null.
     * @param gameBoardPresenter The {@link GameBoardPresenter}. Cannot be null.
     */
    public MemoryGamePresenter(final MemoryGameView view, final UserPanelPresenter userPanelPresenter,
            final ChatPanelPresenter chatPanelPresenter, final GameBoardPresenter gameBoardPresenter) {
        if (view == null) {
            throw new IllegalArgumentException("view cannot be null");
        }
        if (userPanelPresenter == null) {
            throw new IllegalArgumentException("userPanelPresenter cannot be null");
        }
        if (chatPanelPresenter == null) {
            throw new IllegalArgumentException("chatPanelPresenter cannot be null");
        }
        if (gameBoardPresenter == null) {
            throw new IllegalArgumentException("gameBoardPresenter cannot be null");
        }
        this.view = view;
        this.userPanelPresenter = userPanelPresenter;
        this.chatPanelPresenter = chatPanelPresenter;
        this.gameBoardPresenter = gameBoardPresenter;
    }

}

MemoryGameView

/**
 * Provides the UI elements for the base game view.
 */
public class MemoryGameView {

    private final UserPanelView userPanelView;
    private final ChatPanelView chatPanelView;
    private final GameBoardView gameBoardView;

    /**
     * Constructor
     * @param width The width of the game window. Must be a positive number.
     * @param height The height of the game window. Must be a positive number.
     * @param title The title of the game window. Cannot be null.
     * @param theme The {@link MemoryGameTheme}. Cannot be null.
     */
    public MemoryGameView(final int width, final int height, final String title, final MemoryGameTheme theme) {
        if (width <= 0) {
            throw new IllegalArgumentException("width must be positive");
        }
        if (height < -0) {
            throw new IllegalArgumentException("height must be positive");
        }
        if (title == null) {
            throw new IllegalArgumentException("title cannot be null");
        }
        if (theme == null) {
            throw new IllegalArgumentException("theme cannot be null");
        }

        final JFrame frame = new JFrame(title);
        final Container basePane = frame.getContentPane();

        final JPanel gameBoardPanel = new JPanel();
        basePane.add(gameBoardPanel, BorderLayout.CENTER);

        final JPanel userPanel = new JPanel();
        basePane.add(userPanel, BorderLayout.EAST);

        final JPanel chatPanel = new JPanel();
        basePane.add(chatPanel, BorderLayout.SOUTH);

        this.userPanelView = new UserPanelView(userPanel);
        this.chatPanelView = new ChatPanelView(chatPanel);
        this.gameBoardView = new GameBoardView(gameBoardPanel, theme);

        frame.setDefaultCloseOperation(JFrame.DISPOSE_ON_CLOSE);
        frame.pack();
        frame.setSize(width, height);
        frame.setLocationRelativeTo(null);
        frame.setResizable(false);
        frame.setVisible(true);
    }

    /**
     * Gets the {@link UserPanelView}.
     * @return The non-null {@link UserPanelView}.
     */
    public UserPanelView getUserPanelView() {
        return userPanelView;
    }

    /**
     * Gets the {@link ChatPanelView}.
     * @return The non-null {@link ChatPanelView}.
     */
    public ChatPanelView getChatPanelView() {
        return chatPanelView;
    }

    /**
     * Gets the {@link GameBoardView}.
     * @return The non-null {@link GameBoardView}.
     */
    public GameBoardView getGameBoardView() {
        return gameBoardView;
    }

}

GameBoardPresenter

/**
 * Responsible for responding to user interaction and updating the
 * {@link GameBoardView}.
 */
public class GameBoardPresenter implements GameBoardViewListener {

    private final GameBoardView view;

    /**
     * Constructor
     * @param view The {@link GameBoardView}. Cannot be null.
     */
    public GameBoardPresenter(final GameBoardView view) {
        if (view == null) {
            throw new IllegalArgumentException("view cannot be null");
        }
        this.view = view;
        view.addListener(this);
        //@formatter:off
        // In reality you can make a way to get a random set of pairs of cards :)
        view.setCards(Arrays.asList(
                Card.ACE.name(),
                Card.FOUR.name(),
                Card.QUEEN.name(),
                Card.QUEEN.name(),
                Card.FOUR.name(),
                Card.ACE.name()));
        //@formatter:on
    }

    /**
     * {@inheritDoc}
     */
    @Override
    public void onCardSelected(final String cardName) {
        // Do some logic here (flip the card, is there a match? Then what?)
    }

}

GameBoardView

/**
 * Provides the UI elements for the game board view.
 */
public class GameBoardView {

    private final JPanel cardsPanel;
    private final MemoryGameTheme theme;

    private final List<GameBoardViewListener> listeners;
    private final Map<String, JLabel> cardLabels;

    /**
     * Constructor
     * @param parent The parent {@link JPanel}. Cannot be null.
     * @param theme The {@link MemoryGameTheme}. Cannot be null.
     */
    public GameBoardView(final JPanel parent, final MemoryGameTheme theme) {
        if (parent == null) {
            throw new IllegalArgumentException("parent cannot be null");
        }
        if (theme == null) {
            throw new IllegalArgumentException("theme cannot be null");
        }
        this.theme = theme;

        final JPanel basePanel = new JPanel();

        cardsPanel = new JPanel();
        cardsPanel.setLayout(new GridLayout());
        basePanel.add(cardsPanel, BorderLayout.CENTER);

        parent.setLayout(new GridBagLayout());
        final GridBagConstraints gridBagConstraints = new GridBagConstraints();
        gridBagConstraints.fill = GridBagConstraints.BOTH;
        parent.add(basePanel, gridBagConstraints);

        listeners = new ArrayList<GameBoardViewListener>();
        cardLabels = new HashMap<String, JLabel>();
    }

    /**
     * Adds a {@link GameBoardViewListener}.
     * @param listener The {@link GameBoardViewListener}. Cannot be null.
     */
    public void addListener(final GameBoardViewListener listener) {
        if (listener == null) {
            throw new IllegalArgumentException("listener cannot be null");
        }
        listeners.add(listener);
    }

    /**
     * Resets the game board with the given cards.
     * @param cardNames The {@link List} of cards to display. Cannot be null.
     */
    public void setCards(final List<String> cardNames) {
        if (cardNames == null) {
            throw new IllegalArgumentException("cardNames cannot be null");
        }
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                cardLabels.clear();
                cardsPanel.removeAll();
                cardsPanel.setLayout(new GridLayout(2, (cardNames.size() + 1) / 2, 20, 20));

                for (final String cardName : cardNames) {
                    System.out.println(cardName);
                    final JLabel cardLabel = new JLabel(theme.getCardBackIcon());
                    cardLabel.setName(cardName);
                    cardLabel.addMouseListener(new MouseAdapter() {
                        @Override
                        public void mouseClicked(MouseEvent e) {
                            notifyListenersOnCardSelected(cardLabel.getName());
                        }
                    });
                    cardsPanel.add(cardLabel);
                    cardLabels.put(cardName, cardLabel);
                }

                cardsPanel.revalidate();
                cardsPanel.repaint();
            }
        });
    }

    private void notifyListenersOnCardSelected(final String cardName) {
        for (final GameBoardViewListener listener : listeners) {
            listener.onCardSelected(cardName);
        }
    }

}

GameBoardViewListener

/**
 * Provides methods to respod to user interaction with the {@link GameBoardView}.
 */
public interface GameBoardViewListener {

    /**
     * Method to be called when a card is selected.
     * @param cardName The name of the selected card. Cannot be null.
     */
    public void onCardSelected(final String cardName);

}

ChatPanelPresenter

/**
 * Responsible for responding to user interaction and updating the
 * {@link ChatPanelView}.
 */
public class ChatPanelPresenter implements ChatPanelViewListener {

    private final ChatPanelView view;

    /**
     * Constructor
     * @param view The {@link ChatPanelView}. Cannot be null.
     */
    public ChatPanelPresenter(final ChatPanelView view) {
        if (view == null) {
            throw new IllegalArgumentException("view cannot be null");
        }
        this.view = view;
        view.addListener(this);
    }

    @Override
    public void onSendButtonClicked() {
        // TODO Auto-generated method stub
    }

}

ChatPanelView

/**
 * Provides the UI elements for the chat panel view.
 */
public class ChatPanelView {

    private final List<ChatPanelViewListener> listeners;

    /**
     * Constructor
     * @param parent The parent {@link JPanel}. Cannot be null.
     */
    public ChatPanelView(final JPanel parent) {
        if (parent == null) {
            throw new IllegalArgumentException("parent cannot be null");
        }

        final JPanel basePanel = new JPanel(new BorderLayout());

        final JTextArea messagesTextArea = new JTextArea(10, 10);
        messagesTextArea.setFocusable(false);
        basePanel.add(messagesTextArea, BorderLayout.CENTER);

        final JPanel inputsPanel = new JPanel(new BorderLayout());
        basePanel.add(inputsPanel, BorderLayout.SOUTH);

        final JTextField inputField = new JTextField(10);
        inputField.grabFocus();
        inputsPanel.add(inputField, BorderLayout.CENTER);

        final JButton sendButton = new JButton("Send");
        sendButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                notifyListenersOnSendButtonClicked();
            }
        });
        inputsPanel.add(sendButton, BorderLayout.EAST);

        parent.setLayout(new BorderLayout());
        parent.add(basePanel);

        listeners = new ArrayList<ChatPanelViewListener>();
    }

    /**
     * Adds a {@link ChatPanelViewListener}.
     * @param listener The {@link ChatPanelViewListener}. Cannot be null.
     */
    public void addListener(final ChatPanelViewListener listener) {
        if (listener == null) {
            throw new IllegalArgumentException("listener cannot be null");
        }
        listeners.add(listener);
    }

    private void notifyListenersOnSendButtonClicked() {
        for (final ChatPanelViewListener listener : listeners) {
            listener.onSendButtonClicked();
        }
    }

}

ChatPanelViewListener

/**
 * Provides methods to respond to user interaction with the
 * {@link ChatPanelView}.
 */
public interface ChatPanelViewListener {

    /**
     * Method to be called when the send button is clicked.
     */
    public void onSendButtonClicked();

}

UserPanelPresenter

/**
 * Responsible for responding to user interaction and updating the
 * {@link UserPanelView}.
 */
public class UserPanelPresenter implements UserPanelViewListener {

    private final UserPanelView view;

    /**
     * Constructor
     * @param view The {@link UserPanelView}. Cannot be null.
     */
    public UserPanelPresenter(final UserPanelView view) {
        if (view == null) {
            throw new IllegalArgumentException("view cannot be null");
        }
        this.view = view;
        view.addUser("Player One");
        view.addUser("Player Two");
        view.addUser("Player Three");
        view.addUser("Player Four");
    }

}

UserPanelView

/**
 * Provides the UI elements for the user panel view.
 */
public class UserPanelView {

    private final JPanel usersPanel;

    /**
     * Constructor
     * @param parent The parent {@link JPanel}. Cannot be null.
     */
    public UserPanelView(final JPanel parent) {
        if (parent == null) {
            throw new IllegalArgumentException("parent cannot be null");
        }
        final JPanel basePanel = new JPanel(new BorderLayout());
        basePanel.setBorder(new EmptyBorder(20, 20, 20, 20));
        basePanel.setPreferredSize(new Dimension(200, 100));

        usersPanel = new JPanel();
        final BoxLayout usersPanelLayout = new BoxLayout(usersPanel, BoxLayout.Y_AXIS);
        usersPanel.setLayout(usersPanelLayout);
        basePanel.add(usersPanel, BorderLayout.CENTER);

        parent.setLayout(new BorderLayout());
        parent.add(basePanel, BorderLayout.CENTER);
    }

    /**
     * Adds a user to the view.
     * @param username The username {@link String} to add. Cannot be null.
     */
    public void addUser(final String username) {
        if (username == null) {
            throw new IllegalArgumentException("username cannot be null");
        }
        SwingUtilities.invokeLater(new Runnable() {
            @Override
            public void run() {
                final JLabel userLabel = new JLabel(username);
                usersPanel.add(userLabel);
                usersPanel.revalidate();
                usersPanel.repaint();
            }
        });
    }

}

UserPanelViewListener

/**
 * Provides methods to respond to user interaction with the
 * {@link UserPanelView}.
 */
public interface UserPanelViewListener {
    // TODO Nothing here yet!
}

MemoryGameTheme

/**
 * Class to represent all UI resources.
 */
public class MemoryGameTheme {

    private final Map<String, ImageIcon> cardFaceIcons;
    private final ImageIcon cardBackIcon;

    /**
     * Constructor
     * @param cards The {@link List} of {@link Card}s for which image resources are needed. Cannot be null.
     * @param cardWidth The width to scale image resources to. Must be a positive number.
     * @param cardHeight The height to scale image resources to. Must be a positive number.
     */
    public MemoryGameTheme(final List<Card> cards, final int cardWidth, final int cardHeight) {
        if (cards == null) {
            throw new IllegalArgumentException("cards cannot be null");
        }
        if (cardWidth <= 0) {
            throw new IllegalArgumentException("cardWidth must be a positive number");
        }
        if (cardHeight <= 0) {
            throw new IllegalArgumentException("cardHeight must be a positive number");
        }
        cardFaceIcons = new HashMap<String, ImageIcon>();
        for (Card card : cards) {
            cardFaceIcons.put(card.name(), getScaledCardImage(card.name(), cardWidth, cardHeight));
        }
        cardBackIcon = getScaledCardImage("BACK", cardWidth, cardHeight);
    }

    private ImageIcon getScaledCardImage(final String resourceKey, final int cardWidth, final int cardHeight) {
        final String filename = ResourceBundle.getBundle("IconFilenameBundle").getString(resourceKey);
        return new ImageIcon(
                new ImageIcon(filename).getImage().getScaledInstance(cardWidth, cardHeight, Image.SCALE_SMOOTH));
    }

    /**
     * Gets the {@link ImageIcon} corresponding to a given card name.
     * @param cardName The card name. Cannot be null.
     * @return The non-null {@link ImageIcon}.
     */
    public ImageIcon getCardFaceIconForName(final String cardName) {
        if (cardName == null) {
            throw new IllegalArgumentException("cardName cannot be null");
        }
        return cardFaceIcons.get(cardName);
    }

    /**
     * Gets the {@link ImageIcon} for the back of the cards.
     * @return The non-null {@link ImageIcon}.
     */
    public ImageIcon getCardBackIcon() {
        return cardBackIcon;
    }

}

Card

/**
 * Models the different cards that can be shown in the game.
 */
public enum Card {

    TWO, THREE, FOUR, FIVE, SIX, SEVEN, EIGHT, NINE, TEN, JACK, QUEEN, KING, ACE;

}

IconFilenameBundle.properties

BACK = src/main/resources/images/back.jpg
TWO = src/main/resources/images/2.png
THREE = src/main/resources/images/3.png
FOUR = src/main/resources/images/4.png
FIVE = src/main/resources/images/5.png
SIX = src/main/resources/images/6.png
SEVEN = src/main/resources/images/7.png
EIGHT = src/main/resources/images/8.png
NINE = src/main/resources/images/9.png
TEN = src/main/resources/images/10.png
JACK = src/main/resources/images/jack.png
QUEEN = src/main/resources/images/queen.png
KING = src/main/resources/images/king.png
ACE = src/main/resources/images/ace.png

Note: IconFilenameBundle.properties located in src/main/resources

\$\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.