Is it a good practice just to import everything so I don't have to go back and add stuff?
In a modern IDE (e.g. NetBeans) you can just type in the class you intend to use and quick-fix the import. I usually do this so that I don't have spare imports laying around inadvertently. Also, you can use the source cleanup tool to remove unused imports when you're done. I do this in Eclipse, but I'm exceptionally sure that a similar option is in NetBeans. Importing only what you need reduces compilation time.
Is it bad just to let NetBeans do serialVersionUID for me? (I don't like yellow squiggles!)
Do you plan on "serializing" your class? Do you expect anyone else to? If so, you'll want that property to be set, so that different versions of your code can determine which version of the class you're deserializing to (or generate an error). You can use 1L for the first version, and increment it every time you add, remove, or change a member of the class. Or, you can just save that for last if you don't expect many changes when the class is done, and generate it then. If you really don't care about serializing the objects, just suppress the warning with an annotation.
Are those anonymous classes smart or unnecessarily confusing?
The general rule of thumb is: if it's less than (approximately) 10 lines of code, and not frequently used (e.g. is used only once), than anonymous classes are perfectly acceptable, and in fact, encouraged. For code that is bound to be reused often, a named class should be preferred. The Java designers knew that there would often be small classes that only had a few lines of code that, in practice, wouldn't make sense to have in their own source files when one could simply inline the class definition and instantiation at once. Also, because anonymous classes share scope with the outer class, they can get access to the necessary variables and methods without having to accept an instance of the class, modifying member access modifiers, etc.
Am I instantiating more than I need to?
There's no particular excess here, although this may not be the most efficient method you could have come up with. Unless you were under specific direction from a higher authority not to use certain constructs, such as BufferedImages, this almost qualifies as object abuse; components are generally meant to be a way to interact with users, not to just draw arbitrary objects. However, since we're only talking about two frames, it doesn't really matter, and I don't think it would be worth debating the merit of using components versus images.
Is this the best way to handle graphics/Graphics?
Normally, one should create a sort of "canvas", and draw onto that. Here, you're changing the entire content of the window just to flop graphics around. Since there's only two frames, this works out okay, but in a game running at 60FPS, your players would get a headache and/or seizures. You could just as easily have made two BufferedImage elements, and flipped between the two, using a common component that you could draw to.
Do all those fields have to be final?
Fields are final because you want to force certain behavior (e.g. a class cannot be inherited, a value can only be set once, etc). Assuming this was a module that you'd distribute to other developers to use, you'd want anything that should be locked down to be final. You might also use final to prevent yourself from writing code that accidentally modifies the variables. In short, it is a good practice, but it doesn't make sense logically in this example, because this program is sufficiently small enough that you wouldn't likely introduce such a logical error. I would recommend using final as frequently as necessary, but not necessarily here.
Should I make all of the objects final that don't need to change?
Like before, it doesn't really matter in this example. You make values final so as to avoid accidental or intentional changes to the values. Also remember that final can affect inheritance, so if this were a module to a larger application, you might not want to lock everything down with final. In this case, most everything could be final just to prove to yourself that your code isn't making any unintentional changes. I would actually recommend having everything not be final, and for good measure, not static either.
I tried to implement a Circle factory singleton in order not to repeat code. Are there problems? Is this worth it?
In a larger program, factories make sense. In this case, it makes no sense at all; the code needed to support the factory is larger than the savings of not having a factory at all. Consider this version (which does the same thing):
for (double i : circles) {
g2.setColor(colors[count++]);
g2.draw(new Ellipse2D.Double(200 - i / 2, 200 - i, i, i)));
}
It is perfectly legal, and in fact desirable, to instantiate anonymous, short-lived objects that are passed directly into the parameter list of the function, which is essentially what you're doing anyways. This lets people know, clearly, that you're simply creating a circle, instead of having to refer to the factory's source code to get an answer as to what is going on.
In general, are there ways to make my code more succinct?
I believe I've outlined most of the problems in the prior answers, but let's go over them:
- Drop the use of the factory, it's simply overkill in this case.
- Drop the serial ids, especially in anonymous inner classes, as it is unlikely you'll ever serialize them. Warnings are okay, but you can use suppression annotations if you don't like the yellow squiggles (and I personally hate them; they're a guilt trip).
- Use just one component to draw onto (or, one to manage the current view state, if you prefer).
- Don't hide then show the frame. Instead, call invalidate() to force a repaint. No flickering.
- Loading graphics and stuff as an initialization generally makes sense, but not here, because you're just wasting memory (although, not a ton, since this program only weighs in at a few KB, but, the principal remains). You should defer generating the two components until the last possible moment.
- Generally speaking, your main app can be the JFrame; this is a normal design.
Without switching to images and all that, I can simply rearrange your code to the far simpler:
import javax.swing.*;
import java.awt.*;
import java.awt.geom.*;
public class JComponentDemo extends JFrame {
public JComponentDemo() {
super("Drawing");
}
public void start() {
setDefaultCloseOperation(EXIT_ON_CLOSE);
setBounds(50, 50, 500, 500);
setVisible(true);
setContentPane(
new JComponent() {
@Override
public void paintComponent(Graphics g) {
Graphics2D g2 = (Graphics2D) g;
g2.draw(new Line2D.Double(180, 100, 100, 0));
g2.draw(new Line2D.Double(20, 100, 100, 0));
g2.draw(new Line2D.Double(180, 100, 20, 100));
}
});
JOptionPane.showMessageDialog(null, "Click OK to Continue");
setContentPane(new JComponent() {
@Override
public void paintComponent(Graphics g) {
Graphics2D g2 = (Graphics2D) g;
double[] circles = {40, 80, 120, 160, 200};
Color[] colors = {Color.red, Color.green, Color.orange, Color.MAGENTA, Color.cyan};
int count = 0;
for (double i : circles) {
g2.setColor(colors[count++]);
g2.draw(new Ellipse2D.Double(200 - i / 2, 200 - i, i, i));
}
}
});
setVisible(true);
}
public static void main(String[] args) {
new JComponentDemo().start();
}
}
Note that anonymous inner classes are only good for "small" things, so this design wouldn't scale well (imagine a UI with hundreds of components). Regardless, you would still want to instantiate those elements either in a constructor, or in a thread that allocates the objects as the program is starting up or running so that the elements are available when needed.
