3
\$\begingroup\$

This is the basic structure I usually end up with whenever I'm writing a simple application. More recently I've learned about singletons and have started to incorporate them into my design, but other that that I haven't changed much about it in recent times.

I've been wondering if there is anything in here that I could improve on or that is considered bad practice.

Main

import javax.swing.*;

public class Main {
    public static void main(String[] args) {
        try {
            UIManager.setLookAndFeel(UIManager.getSystemLookAndFeelClassName());
        } catch (ClassNotFoundException | InstantiationException | UnsupportedLookAndFeelException | IllegalAccessException e) {
            e.printStackTrace();
        }

        Controller.getInstance();
    }
}

Controller

import javax.swing.*;

public class Controller {
    private static Controller instance;

    private static final int FPS = 30;
    public static final int WIDTH = 600;
    public static final int HEIGHT = 800;

    private final JFrame frame;

    private Timer timer;

    public static Controller getInstance() {
        return instance != null ? instance : (instance = new Controller());
    }

    private Controller() {
        //Do setup
        frame = new AppFrame();

        timer = new Timer(1000/FPS, e -> update());
        timer.start();
    }

    private void update() {
        //Update model
        frame.repaint();
    }
}

AppFrame

import javax.swing.*;

public class AppFrame extends JFrame {
    public AppFrame() {
        super("My Example App");
        setResizable(false);
        setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

        add(new AppPanel());

        //Do more setup if applicable

        pack();
        setLocationRelativeTo(null);
        setVisible(true);
    }
}

AppPanel

import javax.swing.*;
import java.awt.*;

public class AppPanel extends JPanel {
    private final Dimension DIMENSION = new Dimension(Controller.WIDTH, Controller.HEIGHT);

    @Override
    public Dimension getPreferredSize() {
        return DIMENSION;
    }

    @Override
    protected void paintComponent(Graphics g) {
        Graphics2D g2 = (Graphics2D) g;

        //Draw things on g2
    }
}
\$\endgroup\$

1 Answer 1

Reset to default
3
\$\begingroup\$

The code looks decent enough. I'd like to point out three issues, though, all related to timing/multi-threading.

GUI → EDT

All GUI operations should happen on the Event Dispatch Thread. Java boots into the Main thread, which is different from the EDT. Your Controller.getInstance() should be called on the EDT:

EventQueue.invokeLater(Controller::getInstance)

Controller.instance should be guarded

Currently, the instance if filled in via the Main thread. If the GUI (on the EDT or through the timer) later requests the instance, there is no guarantee they will see it, or they might see it in an inconsistent/incomplete state.

This can be solved by either making instance volatile or, perhaps better, making getInstance() a synchronized method.

repaint() is on a best-effort basis

It looks like this is the basis for a game or other graphically heavy application (judging from FPS). Swing (and its timers) may not give you the timing guarantees you'd need for things like game logic.

Consider using a game loop with a Canvas and a BufferStrategy, disabling redraw requests, and taking over from there.

Isn't the Timer good enough? I'm tempted to say, yes, it might be good enough, if the game logic is simple and not intense. Though you're no longer blocking the EDT with Thread.sleep (big win), the actions that the timer calls are still executed on the EDT. This makes handling GUI elements and fields much simpler (no sync needed), but heavier stuff may result in jitter. Worse: heavy stuff on the EDT may make your game loop jitter.

In short: the timer is simpler and probably good enough. The game loop in a separate thread (say, your main thread) is more heavyweight but also more reliable and more likely to benefit from hardware acceleration.

Nitpicking

  • Controller.getInstance seems like an odd method to call from main to kick things off. Maybe make it Controller.run() or Controller.getInstance().run() or some such that would better imply the side effect of running your program.

  • getPreferredSize returns a mutable final variable (Dimension is mutable). Perhaps you could try this:

    public Dimension getPreferredSize() {
    return isPreferredSizeSet() ? super.getPreferredSize() : new Dimension(DIMENSION);
}
\$\endgroup\$
4
  • \$\begingroup\$ Thank you, exactly what I was looking for. On the game loop issue: I've asked about this very issue and I was told to use a Swing Timer to update the view. I will look into this some more. On the Singleton initialization: Is it common to have a seperate method to initialize the singleton instance? When I was reading about Singletons, the examples (as far as I remember) showed lazy initialization similar to what I did. \$\endgroup\$ Commented Mar 16, 2016 at 22:20
  • 2
    \$\begingroup\$ @Marv Singleton: the creation is not an issue—barring synchronisation in your particular case. My remark is about how creating the instance ends up showing a window, which I find unexpected. \$\endgroup\$ Commented Mar 16, 2016 at 22:33
  • \$\begingroup\$ Ah, got it. Fair enough. \$\endgroup\$ Commented Mar 16, 2016 at 22:35
  • \$\begingroup\$ @Marv Swing Timer: it's a possible approach, and it will work, but I'd advise against it since events fired from the Swing timer still occur on the EDT, possibly clogging it up and throwing off other timing. Using a dedicated thread for the game loop will offload the calculating and game logic from the EDT, but (big but) it also makes things more complicated. (I'll elaborate in the answer.) \$\endgroup\$ Commented Mar 16, 2016 at 22:39

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.