Problem
I have this project that should respond to events. Can my code can be refactored and become shorter? Am I using actionListener properly?
public class DifferentShapes extends JPanel implements Shapes{
protected JMenuBar menuBar = new JMenuBar();
protected JRadioButton small = new JRadioButton("small");
protected JRadioButton medium = new JRadioButton("medium");
protected JRadioButton large = new JRadioButton("large");
protected Color color = Color.BLACK;
protected Graphics shape;
protected int display = -1;
protected int colorNo = -1;
int divider2 = 2;
int divider4 = 4;
public DifferentShapes()
{
JMenu shapes = new JMenu("Shapes");
JMenuItem circle = shapes.add(new JMenuItem("circle"));
circle.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
display = 0;
repaint();
}
});
JMenuItem square = shapes.add(new JMenuItem("square"));
square.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
display = 1;
repaint();
}
});
JMenuItem triangle = shapes.add(new JMenuItem("triangle"));
triangle.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
display = 2;
repaint();
}
});
JMenu colors = new JMenu("Colors");
JMenuItem red = colors.add(new JMenuItem("red"));
red.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
colorNo = 0;
color = Color.RED;
repaint();
}
});
JMenuItem blue = colors.add(new JMenuItem("blue"));
blue.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
colorNo = 1;
color = Color.BLUE;
repaint();
}
});
JMenuItem yellow = colors.add(new JMenuItem("yellow"));
yellow.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
colorNo = 2;
color = Color.YELLOW;
repaint();
}
});
menuBar.add(shapes);
menuBar.add(colors);
ButtonGroup buttonGroup = new ButtonGroup();
buttonGroup.add(small);
buttonGroup.add(medium);
buttonGroup.add(large);
this.add(small);
this.add(medium);
this.add(large);
large.setSelected(true);
small.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
divider2 = 4;
divider4 = 6;repaint();
}
});
medium.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
divider2 = 3;
divider4 = 5;repaint();
}
});
large.addActionListener(new ActionListener(){
@Override
public void actionPerformed(ActionEvent ae) {
divider2 = 2;
divider4 = 4;repaint();
}
});
}
public static void main(String[] args)
{
Runnable runnable = new Runnable(){
@Override
public void run() {
DifferentShapes panel = new DifferentShapes();
JFrame frame = new JFrame("Different Shapes");
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
frame.setSize(400,400);
frame.setVisible(true);
frame.add(panel);
frame.setJMenuBar(panel.getMenuBAr());
}
};EventQueue.invokeLater(runnable);
}
@Override
public void paintComponent(Graphics shape)
{
super.paintComponent(shape);
switch(colorNo)
{
case 0 : shape.setColor(color);break;
case 1 : shape.setColor(color);break;
case 2 : shape.setColor(color);break;
}
switch(display)
{
case 0 : circle(shape);break;
case 1 : square(shape);break;
case 2 : triangle(shape);break;
}
}
//4 4 2 2
@Override
public void circle(Graphics shape) {
shape.fillOval(getWidth()/divider4, getHeight()/divider4, getWidth()/divider2, getWidth()/divider2);
}
@Override
public void square(Graphics shape) {
shape.fillRect(getWidth()/divider4, getHeight()/divider4, getWidth()/divider2, getHeight()/divider2);
}
@Override
public void triangle(Graphics shape) {
int[] x = new int[3];
int[] y = new int[3];
x[0] = getWidth() / 4;
x[1] = getWidth() / 2;
x[2] = 3 * getWidth() / 4;
y[0] = 3 * getHeight() / divider4;
y[1] = getHeight() / divider4;
y[2] = y[0];
Polygon polygon = new Polygon(x, y, 3);
shape.fillPolygon(polygon);
}
private JMenuBar getMenuBAr() {
return menuBar;
}
}
Solution
One way to shorten your code is by changing the menu initialization in the DifferentShapes constructor (while using a Java 8 lambda expression):
JMenu shapes = new JMenu("Shapes");
List<String> shapeList = Arrays.asList("circle", "square", "triangle");
ActionListener shapeActionListener = actionEvent -> {
String shape = ((JMenuItem) actionEvent.getSource()).getText();
if (shapeList.contains(shape)) {
display = shapeList.indexOf(shape);
repaint();
}
};
for (String shape : shapeList) {
JMenuItem menuItem = shapes.add(new JMenuItem(shape));
menuItem.addActionListener(shapeActionListener);
}
By using a list of the shape names and sharing the action listener, you can rewrite around 28 lines to 14 lines. You could use a similar approach for the colors and sizes.
And two general remarks:
- Consider making the fields private.
- I would add a
scaleFactorfield (0.5, 0.33, or 0.25) and replace the/ divider2and/ divider4occurrences by* scaleFactorand* (0.5 * scaleFactor). This would in my opinion be clearer and give almost the same results.
Edit: Java 7 action listener
For Java 7, you can create an action listener with an anonymous inner class like this:
ActionListener shapeActionListener = new ActionListener() {
@Override
public void actionPerformed(ActionEvent actionEvent) {
String shape = ((JMenuItem) actionEvent.getSource()).getText();
if (shapeList.contains(shape)) {
display = shapeList.indexOf(shape);
DifferentShapes.this.repaint();
}
}
};
Since July 2015, Oracle is no longer making updates available to the public for Java 7 and is suggesting to migrate to Java 8. You could also upgrade to the latest version of NetBeans (which is NetBeans 8.1 in January 2016).