Sure, be my guest. I always look forward to learning how to code better
In that case, here goes!
First off, I'm not sure how much experience you have with either / or Swing and threading? There's a few rules that need to be followed to guarantee that your application will be free of threading bugs, and the main one in this instance is that you must only ever do anything to Swing components if you're running on the event dispatch thread (see here for details.)
Granted, some Swing objects *are* thread safe, but for the moment assume that none are - it's considered good practice to just do everything that needs to be done in Swing on the EDT.
In your case, the update() method is called from an arbitrary thread which is most definitely *not* the EDT (it's one that you've just created), so this is especially dangerous since the two are running concurrently. (No, you might not usually run into these problems with simple applications, but for more advanced ones you definitely will at some point, so pays to have it right straight off.)
If you're not on the EDT but you want to execute a piece of Swing code on the EDT, then it's relatively easy to do:
Code:
SwingUtilities.invokeLater(new Runnable() {
public void run() {
//Code to run on the EDT goes here.
}
});This essentially schedules a runnable to be executed on the EDT - it will be invoked at some point in the near future. If you want to wait until that drawing code is complete (which isn't usually what you want but is available anyway) then use invokeAntWait() rather than invokeLater(). As you've probably already found, if you attempt to do *everything* on the EDT then your application will just appear to lock up while these tasks are completing, not what you want.
You may be starting to realise there's more to threading than you first thought - it's a hugely complicated topic, entire books have been written on threading issues you can come across just in Java (Java concurrency in practice is a great book if you're interested, but not for the beginner.) Introducing threading to your application unlocks a whole potential hive of livelocks, deadlocks, race hazards and so on, and many will be unpredictable and hard to track down. So use threads wisely, and only when you have to
In terms of other points, overall it's not bad but I'd suggest a few style / misc changes:
- Comments are a bit sparodic - you've put one about declaring the variables, but this isn't really needed (it's obvious to anyone reading the code that's what you're doing.) However, you should really comment all methods and explain what they do, because that isn't so obvious. In these comments you can put things like whether the method needs to be executed on the EDT or not in order to aid others that may be using your code. Might not be important now, but in a team of people or on a large codebase good commenting gets incredibly important. Use the set Javadoc style for this.
- Your imports are a bit sparodic - you've got one star one and the rest are just individual imports. I'd advocate using individual imports for everything, since it makes it clearer to the reader what you're importing and mitigates name clashes (two classes with the same name in different packages you may be importing.) Whatever you choose, make it consistent.
- A lot of things are static that shouldn't be static - unless you can really justify it, nothing should really be static (at least tell yourself that to start with.) I see nothing in that class that should really be static.
- With fields such as "private static ArrayList<JLabel> labels = new ArrayList<JLabel>();" - first off you don't need the second JLabel class, you can use diamond, and secondly it's better practice to use an interface type for the field rather than tying it to an implementation - it makes things much easier to swap out that way. So it would become: "private static List<JLabel> labels = new ArrayList<>();" (where list is java.util.List.)
So overall, it's not bad - but there are definitely things you can work on even if you want to just keep the same level of functionality!
Last edited:
