1
\$\begingroup\$
    public void actionPerformed(ActionEvent e) {
        Object source = e.getSource();
        if (source.equals(jbColor1))
            color = getBlue();
        else if (source.equals(jbColor2))
            color = getRed();
        else if (source.equals(jbColor3))
            color = getGreen();
        else if (source.equals(jbColor4))
            color = getBlack();
        else if (source.equals(jbColor5))
            color = getYellow();
        else if (source.equals(jbColor6))
            color = getPink();
        else if (source.equals(btnRectangle)) {
            isSelectedReactangle = true;
            isSelectedCircle = false;
            isSelectedLine = false;
            btnRectangle.setBorder(BorderFactory.createLineBorder(Color.BLACK, 3));
            btnLine.setBorder(UIManager.getBorder(defaultbtnLable));
            btnCircle.setBorder(UIManager.getBorder(defaultbtnLable));
            count = 0;
        } else if (source.equals(btnCircle)) {
            isSelectedReactangle = false;
            isSelectedCircle = true;
            isSelectedLine = false;
            btnCircle.setBorder(BorderFactory.createLineBorder(Color.BLACK, 3));
            btnLine.setBorder(UIManager.getBorder(defaultbtnLable));
            btnRectangle.setBorder(UIManager.getBorder(defaultbtnLable));
            count = 0;
        } else if (source.equals(btnLine)) {
            isSelectedReactangle = false;
            isSelectedCircle = false;
            isSelectedLine = true;
            btnLine.setBorder(BorderFactory.createLineBorder(Color.BLACK, 3));
            btnRectangle.setBorder(UIManager.getBorder(defaultbtnLable));
            btnCircle.setBorder(UIManager.getBorder(defaultbtnLable));
            count = 0;
        } else if (source.equals(btnClear)) {
            btnLine.setBorder(UIManager.getBorder(defaultbtnLable));
            btnRectangle.setBorder(UIManager.getBorder(defaultbtnLable));
            btnCircle.setBorder(UIManager.getBorder(defaultbtnLable));
            panelContent.repaint();
        }
}

I have some code but i would avoid if else statement. for example this snippet code is repeating few times.

btnLine.setBorder(BorderFactory.createLineBorder(Color.BLACK, 3));
btnRectangle.setBorder(UIManager.getBorder(defaultbtnLable));
btnCircle.setBorder(UIManager.getBorder(defaultbtnLable));

Some tips and tricks how to avoid if condition so many times

\$\endgroup\$
5
  • 1
    \$\begingroup\$ Welcome to Code Review! Your original title was too generic to be appropriate for this site. I had to take a guess at what this code does. If you could provide more context about your goals, we could provide better advice. See How to Ask. \$\endgroup\$ Oct 16 '17 at 12:54
  • \$\begingroup\$ Can you show me those getPink, getBlack etc methods? \$\endgroup\$ Oct 16 '17 at 16:13
  • \$\begingroup\$ @Simon Forsberg for example private Color getGreen() { return Color.GREEN; } Other methods are analogous \$\endgroup\$ Oct 16 '17 at 16:19
  • \$\begingroup\$ @WalterWhite With methods that simple, why have them as methods at all? \$\endgroup\$ Oct 16 '17 at 16:21
  • \$\begingroup\$ @SimonForsberg I'm using this code in few place and I think this should be good if i use method but You are right this method are so simple. \$\endgroup\$ Oct 16 '17 at 16:24
1
\$\begingroup\$

Why would you have one actionPerformed() method for multiple UI elements instead of creating each button with its individual Action instance? Like:

Action clearAction = new AbstractAction("Clear") {
        public void actionPerformed(ActionEvent e) {
            btnLine.setBorder(UIManager.getBorder(defaultbtnLable));
            btnRectangle.setBorder(UIManager.getBorder(defaultbtnLable));
            btnCircle.setBorder(UIManager.getBorder(defaultbtnLable));
            panelContent.repaint();
        }
}
btnClear = new JButton(clearAction);

[Disclaimer: may contain typos, didn't check it in an IDE]

Then the Action is a first-class object, can be bound to multiple UI elements (e.g. a button and a main menu item) and contains both the name (plus an icon if you like) and the implementation.

And if the Actions have common elements, maybe it's worth having them inherit from a common base Action class.

\$\endgroup\$
1
  • \$\begingroup\$ Yes, common base Action classes are very useful. In fact, most often they don't need to inherit from those, but can instead be an instance of those by passing the right arguments to the constructor. \$\endgroup\$ Oct 16 '17 at 16:15
0
\$\begingroup\$

You might be able to put the code into a function:

void setSettings(YourClass object, boolean isSelectedReactangle, boolean isSelectedCircle, boolean isSelectedLine) {
    object.isSelectedReactangle = false;
    object.isSelectedCircle = true;
    object.isSelectedLine = false;
    btnCircle.setBorder(BorderFactory.createLineBorder(Color.BLACK, 3));
    btnLine.setBorder(UIManager.getBorder(defaultbtnLable));
    btnRectangle.setBorder(UIManager.getBorder(defaultbtnLable));
    count = 0;
}

You would still probably have to use the if/else, as switch statements can only switch on limited types of objects.

Also, Reactangle should probably be Rectangle.

\$\endgroup\$

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.