2
\$\begingroup\$

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;
}
}
\$\endgroup\$
  • \$\begingroup\$ I removed the request for additional features. Please, edit your question and title, to better describe what the purpose of the code is. \$\endgroup\$ – Mathieu Guindon Jan 28 '16 at 18:20
  • \$\begingroup\$ soo... what java version are you working on? \$\endgroup\$ – Vogel612 Jan 28 '16 at 18:26
  • \$\begingroup\$ Where does the Shapes interface come from that the DifferentShapes class implements? \$\endgroup\$ – Freek de Bruijn Jan 28 '16 at 20:49
  • \$\begingroup\$ @FreekdeBruijn yeah it's a interface but I think it's not neccessary right? \$\endgroup\$ – Jay Gorio Jan 28 '16 at 23:47
  • \$\begingroup\$ @Jay Gorio It can be useful to have an interface if you're planning to add more implementations (other classes). I asked about it because the code of the interface was not included in the question, to make sure I wasn't missing something important. \$\endgroup\$ – Freek de Bruijn Jan 31 '16 at 10:01
1
\$\begingroup\$

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 scaleFactor field (0.5, 0.33, or 0.25) and replace the / divider2 and / divider4 occurrences by * scaleFactor and * (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).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Im using netbeans 7 \$\endgroup\$ – Jay Gorio Jan 28 '16 at 23:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.