7
\$\begingroup\$

I have this painting app called JPaint in Java and I'm wondering if it could be improved. The variable declaration seems repetitive and long, and I think it might be able to be improved.

public class JPaint3 {

    private JFrame frame;
    private JFrame startFrame;
    private DrawPanel drawPanel;
    private LineBorder border;

    private JRadioButton setRed;
    private JRadioButton setBlue;
    private JRadioButton setYellow;
    private JRadioButton setGreen;
    private JRadioButton setOrange;
    private JRadioButton setPurple;
    private JRadioButton setBlack;
    private JRadioButton setCyan;
    private JRadioButton setPink;
    private JRadioButton setErase;
    private JButton clearButton;
    private JButton fillButton;
        private JButton startButton;
    private JRadioButton setSquare;
    private JRadioButton setCircle;
    private JRadioButton setTriangle;

    private Color dotColor = Color.RED;
        private Graphics graphics = null;
    private ShapeType shapeType = ShapeType.CIRCLE;
    private final Color PURPLE = new Color(80, 0, 80);
        private final Color PINK = new Color(255, 20 , 147);
    private Image logo;

    private static JPaint3 paintApp;

    public static void main(String[] args) {
        paintApp = new JPaint3();
        paintApp.start();
    }
    public void go(){

        frame = new JFrame();
        frame.setTitle("JPaint: Version 1.2");
        border = new LineBorder(Color.BLACK, 5, true);
        setRed = new JRadioButton("Red", true);
        setBlue = new JRadioButton("Blue", false);
        setYellow = new JRadioButton("Yellow", false);
        setGreen = new JRadioButton("Green", false);
        setOrange = new JRadioButton("Orange", false);
        setPurple = new JRadioButton("Purple", false);
        setBlack = new JRadioButton("Black", false);
        setPink = new JRadioButton("Pink", false);
        setCyan = new JRadioButton("Cyan", false);
        setErase = new JRadioButton("Erase", false);
        clearButton = new JButton("Clear Screen");
        fillButton = new JButton("Fill Screen");
        setSquare = new JRadioButton("Square", false);
        setCircle = new JRadioButton("Circle", true);
        setTriangle = new JRadioButton("Triangle", false);

        setRed.addItemListener(new ColorListener(Color.RED));
        setBlue.addItemListener(new ColorListener(Color.BLUE));
        setYellow.addItemListener(new ColorListener(Color.YELLOW));
        setGreen.addItemListener(new ColorListener(Color.GREEN));
        setOrange.addItemListener(new ColorListener(Color.ORANGE));
        setPurple.addItemListener(new ColorListener(PURPLE));
        setBlack.addItemListener(new ColorListener(Color.BLACK));
        setPink.addItemListener(new ColorListener(PINK));
        setCyan.addItemListener(new ColorListener(Color.CYAN));
        setErase.addItemListener(new EraseListener());

        setSquare.addItemListener(new ShapeListener(ShapeType.SQUARE));
        setCircle.addItemListener(new ShapeListener(ShapeType.CIRCLE));
        setTriangle.addItemListener(new ShapeListener(ShapeType.TRIANGLE));

        clearButton.addActionListener(new ClearListener());
        fillButton.addActionListener(new FillListener());

        ButtonGroup colorsGroup = new ButtonGroup();
        colorsGroup.add(setBlue);
        colorsGroup.add(setRed);
        colorsGroup.add(setYellow);
        colorsGroup.add(setGreen);
        colorsGroup.add(setOrange);
        colorsGroup.add(setPurple);
        colorsGroup.add(setBlack);
        colorsGroup.add(setPink);
        colorsGroup.add(setCyan);
        colorsGroup.add(setErase);

        ButtonGroup shapeGroup = new ButtonGroup();
        shapeGroup.add(setSquare);
        shapeGroup.add(setCircle);
        shapeGroup.add(setTriangle);        

        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        Container window = frame.getContentPane();

        drawPanel = new DrawPanel();
        JPanel optionPanel = new JPanel();
        JPanel colors = new JPanel();
        JPanel shapes = new JPanel();
        JPanel bottomPanel = new JPanel();
        JPanel editPanel = new JPanel();

        editPanel.add(clearButton);
        editPanel.add(fillButton);

        colors.add(setRed);
        colors.add(setBlue);
        colors.add(setYellow);
        colors.add(setGreen);
        colors.add(setOrange);
        colors.add(setPurple);
        colors.add(setBlack);
        colors.add(setPink);
        colors.add(setCyan);
        colors.add(setErase);

        shapes.add(setCircle);
        shapes.add(setSquare);
        shapes.add(setTriangle);

        colors.setBackground(Color.YELLOW);
        shapes.setBackground(Color.GREEN);

        optionPanel.add(colors);

        bottomPanel.add(editPanel);
        bottomPanel.add(shapes);

        optionPanel.setBorder(border);
        bottomPanel.setBorder(border);

        optionPanel.setBackground(Color.GREEN);
        editPanel.setBackground(Color.CYAN);
        bottomPanel.setBackground(Color.GREEN);
        shapes.setBackground(Color.CYAN);

        window.add(BorderLayout.NORTH, optionPanel);
        window.add(BorderLayout.SOUTH, bottomPanel);
        window.add(BorderLayout.CENTER, drawPanel);

        frame.setSize(800, 800);
        frame.setVisible(true);
    }

    public void start() {
        startFrame = new JFrame("Let's Begin!");
        try {
            logo = ImageIO.read(this.getClass().getResource("javaLogo.png"));
        } catch (IOException e) {}
        startButton = new JButton("Click to begin");
                JLabel label = new JLabel("       JPaint!!!      ");
        Font fancyFont = new Font("Zapfino", Font.BOLD + Font.ITALIC, 24);
                Font bigFont = new Font("Chalkboard", Font.BOLD, 30);
        startButton.addActionListener(new StartListener());
        startButton.setFont(fancyFont);
                label.setFont(bigFont);
        startButton.setPreferredSize(new Dimension(240, 50));
        JLabel imgLabel = new JLabel(new ImageIcon(logo));
        startFrame.add(BorderLayout.SOUTH, startButton);
                startFrame.add(BorderLayout.CENTER, label);
        startFrame.add(BorderLayout.NORTH, imgLabel);
        startFrame.pack();
                startFrame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        startFrame.setVisible(true);
    }
    private class ColorListener implements ItemListener {
        private final Color color;
        public ColorListener(Color color) {
            this.color = color;
        }
        @Override
        public void itemStateChanged(ItemEvent e) {
            if (e.getStateChange() == ItemEvent.SELECTED)
                dotColor = this.color;
        }

    }
    private class ShapeListener implements ItemListener {
        private final ShapeType type;
        public ShapeListener(ShapeType type) {
            this.type = type;
        }
        @Override
        public void itemStateChanged(ItemEvent e) {
            if (e.getStateChange() == ItemEvent.SELECTED)
                shapeType = this.type;
        }

    }
    private class ClearListener extends FillListener {
        @Override
        public void actionPerformed(ActionEvent e) {

                    Color theDotColor = dotColor;
                    dotColor = Color.WHITE;
                    super.actionPerformed(e);
                    dotColor = theDotColor;
        }

    }
    private class EraseListener extends ColorListener {
        public EraseListener() {
            super(Color.WHITE);
        }
    }
    private class FillListener implements ActionListener {
        @Override
        public void actionPerformed(ActionEvent arg0) {
            Dimension d = frame.getSize();
                        Graphics graphics = drawPanel.getGraphics();
            graphics.setColor(dotColor);
            graphics.fillRect(0, 0, d.width, d.height);
        }
    }
    private class DrawPanel extends JPanel implements MouseListener, MouseMotionListener{

                @Override
        public void paintComponent(Graphics g) {
            graphics = this.getGraphics();
            this.addMouseListener(this);
            this.addMouseMotionListener(this);
        }
        public void drawShape(int x, int y, ShapeType type) {
                    graphics = this.getGraphics();
                    graphics.setColor(dotColor);
                    if (type == ShapeType.CIRCLE)
                        graphics.fillOval(x, y, 20, 20);
                    else if (type == ShapeType.SQUARE)
                        graphics.fillRect(x, y, 20, 20);
                    else if (type == ShapeType.TRIANGLE){
                        int[] xValues = {x, x + 10, x + 20};
                        int[] yValues = {y + 20, y, y + 20};
                        graphics.fillPolygon(xValues, yValues, 3);}
                    }

                public void clear() {
                    Dimension d = frame.getSize();
                    Graphics graphics = drawPanel.getGraphics();
                    graphics.setColor(dotColor);
                    graphics.fillRect(0, 0, d.width, d.height);
                }
        @Override
        public void mouseDragged(MouseEvent e) {
                int mouseX = e.getX();
                int mouseY = e.getY();
                drawShape(mouseX, mouseY, shapeType);
        }

        @Override
        public void mouseMoved(MouseEvent arg0) {

        }
        @Override
        public void mouseClicked(MouseEvent e) {
                        int mouseX = e.getX();
            int mouseY = e.getY();
            drawShape(mouseX, mouseY, shapeType);
        }
        @Override
        public void mouseEntered(MouseEvent arg0) {

        }
        @Override
        public void mouseExited(MouseEvent arg0) {

        }
        @Override
        public void mousePressed(MouseEvent arg0) {

        }
        @Override
        public void mouseReleased(MouseEvent arg0) {

        }
    }
    private class StartListener implements ActionListener {
        @Override
        public void actionPerformed(ActionEvent arg0) {
            startFrame.setVisible(false);
            paintApp.go();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ You using Java 8? \$\endgroup\$ – Boris the Spider Apr 19 '15 at 9:36
  • \$\begingroup\$ Yes. @BoristheSpider \$\endgroup\$ – Quintec Apr 19 '15 at 16:10
11
\$\begingroup\$

The code is damn long, so just a few notes:

Use initializer expressions like

private JRadioButton setRed = new JRadioButton("Red", true);

Use static factory methods to save even more like

private JRadioButton setRed = makeColorButton("Red", true, Color.RED);

private static JRadioButton makeColorButton(
        String name, boolean isSet, Color color) {
    JradioButton result = new JRadioButton(name, isSet);
    result.addItemListener(new ColorListener(color));
    return result;
}

Actually, I wouldn't bother with isSet as it gets used just once.

ButtonGroup colorsGroup = new ButtonGroup();
colorsGroup.add(setBlue);

Another thing the factory method can do. Just create the group first and pass it to it like

private ButtonGroup colorsGroup = new ButtonGroup();
private JRadioButton setRed = makeColorButton("Red", colorsGroup, Color.RED);

And yet another such thing:

colors.add(setRed);

This makes the code surely much shorter, however, I'd strongly recommend to keep the code short from the very beginning. Whenever you see, you need to do the same things many times, extract them into a method/class/whatever.


private class DrawPanel extends JPanel implements MouseListener, MouseMotionListener{

Use a separate (inner or anonymous) class for the listeners and you can extend MouseAdapter and MouseMotionAdapter and save yourself the empty methods.

\$\endgroup\$
  • \$\begingroup\$ thanks for the suggestions! i will wait a little longer before accepting, to see if anyone else has something else… \$\endgroup\$ – Quintec Apr 18 '15 at 19:15
  • \$\begingroup\$ @thecoder16 Sure, they will. I haven't even read the code (seriously, I stopped at about colors and jumped to the bottom) :D \$\endgroup\$ – maaartinus Apr 18 '15 at 19:35
1
\$\begingroup\$

Some more shortening:

    colorsGroup.add(setBlue);
    colorsGroup.add(setRed);
    colorsGroup.add(setYellow);
    colorsGroup.add(setGreen);
    colorsGroup.add(setOrange);
    colorsGroup.add(setPurple);
    colorsGroup.add(setBlack);
    colorsGroup.add(setPink);
    colorsGroup.add(setCyan);
    colorsGroup.add(setErase);

Becomes:

 for (setter : [setBlue, setRed, setYellow, setGreen,
                setOrange, ..., setErase]) {
     colorsGroup.add(setter);
}
\$\endgroup\$
  • \$\begingroup\$ @thecoder16 Note that this can and should be done in makeColorButton too. \$\endgroup\$ – maaartinus Apr 19 '15 at 16:20
  • \$\begingroup\$ @maartinus Ok. Will do! \$\endgroup\$ – Quintec Apr 19 '15 at 16:20
  • 1
    \$\begingroup\$ @thecoder16 If after you have improved your code you want further suggestions, you can post a follow up question. \$\endgroup\$ – Caridorc Apr 19 '15 at 17:32

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.