This is my first post on this forum. I'm a long time reader though. I am learning up on Swing and concurrency/EDT details, and one of the aspects I've learned is that all Swing operations, including creation, should be done on the EDT. Easy enough. Let me post some code, and ask a question if I may.
You'll notice that I create the primary classes in my main thread, and from that thread I call invokeAndWait() to create the GUI. After that is completed (once it returns), the Controller is created and ran, which adds actionListeners to the radio buttons. All good and well.
On the official documentation, however, Sun mentions that you should prefer invokeLater() over invokeAndWait(). That's fine, but when I use invokeLater(), I get a nullPointerException, because the Controller is attempting to add a listener to a component of the view that hasn't been initialized yet. I feel like I understand the underworkings of everything pretty well, but I have a question on best practices.
Is it allowable(acceptable, kosher, ok) to move the creation of the radio buttons to the constructor? If I did this, they would already be created by the time the action listeners are added by the controller, allowing me to use invokeLater() instead. However, does this violate the rule of "creating" the GUI only on the EDT?
I'm sure there would rarely be a case where either option is a problem, and this is just a simple program for my class (personal extra credit, I don't get any points ), but I am a firm believer that every piece of code you write should be written "correctly", not just "work".
P.S. I removed some trivial code that does not affect the functioning of the program.
P.P.S. If you see other things I may be doing wrong, pointing them out is very much welcomed!
ActionListeners are accessed in the EDT, and any given collection of Listeners is not thread safe. So the addition and removal of listeners should also be performed in the EDT, since that is where they will be accessed and run. So your controller should push the listener assignments into the EDT (with invokeLater()). If you do that, there isn't a chance for the null pointer exception.
Realistically, you don't have to worry about the EDT and thread synchronization until after at least one component is made visible - because that is when the EDT will be started (or the first task is pushed to the EDT via SwingUtilities or the like). So you will often see code which creates the GUI components normally (including adding listeners) and then only pushes the 'frame.setVisible(true)' part into the EDT using SwingUtilities. I am not sure if it is best practice though.
One thing I don't like is that your View and Controller are Runnable and have run() methods. What I think would be better is to remove the Runnable interface. In the View class I would change the run() method to an init() method, and in the Controller change the run() method to assignListeners(). I would do this because that is more descriptive as to the purpose of the method. 'So how do I run those methods, don't I need a Runnable to do that?' you might ask. The answer is - yes, you do, but you shouldn't let that pollute your interface. Instead, you should wrap those methods into a Runnable where the Runnable is a requirement - and you do that with an anonymous inner class based on Runnable:
If you haven't learned about Anonymous Inner Classes yet then you could make another (non-public) class in the same .java file as the View (and a second one in the same .java file as the Contoller) which implements Runnable and does the same thing - just has a run() method which calls the correct method on view/controller. The idea is to keep your public classes that people will see and use from having interfaces which can be confusing. For example, having View implement Runnable would lead someone to believe View itself is an executable task when in fact there is simply a task which needs to be done when it is initializing the components.
One final thing to mention: In your original code in the main method you have a series of exceptions which you catch and handle by printing the stack trace. You then let the application continue to run. If your View doesn't initialize properly does it make sense to continue the program like nothing happened? Maybe you should just end the program with the exceptions message, since there would be no way to continue safely. Or maybe you can think of something to test or do to make sure the view is properly set up. But either way just continuing like it never happened usually just adds further errors down the road - and those ones might not be as easy to track down.
p.s. If you do what I mentioned - removing the run() method from View and Controller, and creating the Runnable as an anonymous inner class, you could make init() and assignListeners() private methods that you call from their respective class' constructors. Having private methods is a good thing - it makes sure those methods don't get called accidentally from external sources when they aren't supposed to. It is another good practice - make all your methods and members with the tightest possible accessibility. This helps in defensive programming, and makes for more stable code since you have tighter control over who can call that method, and when.
If you use the 'make another (non-public) class in the same .java file' option, then you would need to make the init() and assignVariables() methods have at least default accessibility.
Oh, just noticed an error in the above code. I have the init() method 'public static void' but it shouldn't be static - that was a mistake.
Joined: Sep 08, 2012
@Steve Luke <------ Thank you! This helps a lot;
I didn't realize that ActionListeners were also part of the EDT. Putting them in their own method and running them with invokeLater makes perfect sense, allowing me to set GUI creation in the queue first, then add actionlisteners next. Thank you for the advice as well regarding runnable interfaces. I wasn't sure about that, and your solution makes more sense to me now that I see it. As far as the exceptions, I simply hadn't caught them yet, I only added them because I had to use invokeAndWait() which requires those to be caught.
I have another question regarding Graphics (mainly I don't really know how they work) but I will delegate that to another thread. Thank you for the tips. I will fix my code, and post other snippets for the Graphics question.