Drawing a triangle and some concentric circles

For class, I had to use Java to draw a triangle and some concentric circles:

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

public class JComponentDemo {

private static final JComponent triangleComponent = new JComponent() {
private static final long serialVersionUID = 1L;

@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));
}
};

private static final JComponent ConcentricCircleMaker = new JComponent() {
private static final long serialVersionUID = 1L;

@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(Factory.get().makeCircle(200 - i / 2, 200 - i, i));
}
}
};

public static void main(String[] args) {
JFrame frame = new JFrame("Drawing");
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.setBounds(50, 50, 500, 500);
frame.setVisible(true);
frame.setContentPane(triangleComponent);
JOptionPane.showMessageDialog(null, "Click OK to Continue");
frame.setVisible(false);
frame.setContentPane(ConcentricCircleMaker);
frame.setVisible(true);
}

private static class Factory {

private static final FactorySingle factory;

static {
factory = new FactorySingle();
}

static FactorySingle get() {
return factory;
}

private static class FactorySingle {

public E

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 functi

StackExchange Code Review Q#41770, answer score: 11