HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

JPaint (Java painting app)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
jpaintpaintingjavaapp

Problem

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", fal

Solution

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.

Code Snippets

private JRadioButton setRed = new JRadioButton("Red", true);
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;
}
ButtonGroup colorsGroup = new ButtonGroup();
colorsGroup.add(setBlue);
private ButtonGroup colorsGroup = new ButtonGroup();
private JRadioButton setRed = makeColorButton("Red", colorsGroup, Color.RED);
colors.add(setRed);

Context

StackExchange Code Review Q#87304, answer score: 11

Revisions (0)

No revisions yet.