3
\$\begingroup\$

How can I avoid so many if statements?

//Radio Button toggle


if (five.isSelected() == true) {
            one.setSelected(false);

    private class JButtonListner implements ActionListener {

//As my code has 4 options (1,5,+,-) to change the label, i have done 4 if else statments. is it possible to avoid such mess?


        public void actionPerformed(ActionEvent e) {
            if (e.getSource().equals(plus) && five.isSelected() == true) {
                count += 5;
                label.setText(Integer.toString(count));

            } else if (e.getSource().equals(minus) && five.isSelected() == true) {
                count -= 5;
                label.setText(Integer.toString(count));

            } else if (e.getSource().equals(minus) && one.isSelected() == true) {
                count --;
                label.setText(Integer.toString(count));

            } else if (e.getSource().equals(plus) && one.isSelected() == true) {
                count++;
                label.setText(Integer.toString(count));
            }
        }
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to Code Review! I changed the title to a different one that describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. \$\endgroup\$ Nov 23, 2017 at 1:01

3 Answers 3

2
\$\begingroup\$

It would be nice to see more of the code since one, five, plus and minus give NullPointerException

public void actionPerformed(ActionEvent event) {
      int value = five.isSelected() ? 5 : 1;
      if (event.getSource().equals(plus)) {
                count += value;      
      } else if (event.getSource().equals(minus)) {
                count -= value;
      } 
      label.setText(Integer.toString(count));
}

Since you only have 2 RadioButton, that would mean that the value could only be two values, either 5 or 1. This can be done by line two, you are asking if five.isSelected(), if(five.isSelected()) { return 5; } else { return 1; }. Also there is no need to say five.isSelected() == true, because the if already asks if it is true so five.isSelected() is sufficient.

Now you dont need to plus/minus with 5 or 1, but only check plus or minus.

And you set the text each time count has changed, so this can just be done after the if.

\$\endgroup\$
0
\$\begingroup\$

The point is:

If you make the decisions (if-statements) what to do far beyond the elements that make the difference (JButtons) you have no chance to avoid additonal decision constructs.

If the JButton have a 1:1 relationship to the numbers you may store them as "user data" at the JButton (e.g. setActionCommand(...)). Then you may come up with following:

buttonMinus1.setActionCommand("-1");
buttonPlus1.setActionCommand("1");
buttonPlus5.setActionCommand("5");
...

In your listener you then only have one line:

count -= Integer.parseInt(((JButton)e.getSource()).getActionCommand());

This looks a little bit ugly but it is the Swing way. Other frameworks do not work with the String-Type. They often work with the Object-type (Vaadin: button.setData(...), JavaFX: radioButton.setUserData(...), etc.).

Nevertheless: the trick is to make the decision at the origin if possible. In your case in the buttons.

You may also have a look here: https://stackoverflow.com/questions/21533356/is-there-a-way-to-store-data-in-a-jbutton

One big advantage ist:

You are able to add new Buttons (plus 300??) without the need of enhancing any if cascades.

\$\endgroup\$
0
\$\begingroup\$

Unfortunately your code is not complete, so I might make assumptions where something is missing.


if (five.isSelected()) == true) is redundant. Just write if (five.isSelected()) instead.


Be careful with the indentation. Wrong indentation can hurt readability:

    one.setSelected(false);

    private class JButtonListner implements ActionListener {
        public void actionPerformed(ActionEvent e) {

Use local variables to abstract details and increase readability:

            bool plusFive = e.getSource().equals(plus) && five.isSelected();
            bool minusFive = e.getSource().equals(minus) && five.isSelected();
            bool plusOne = e.getSource().equals(plus) && one.isSelected();
            bool minusOne = e.getSource().equals(minus) && one.isSelected();

            if (plusFive) {
                count += 5;
            } else if (minusFive) {
                count -= 5;
            } else if (plusOne) {
                count += 1;
            } else if (minusOne) {
                count -= 1;
            }

You only need to write this once, as it is done in all of the above cases.

            label.setText(Integer.toString(count));
\$\endgroup\$
1
  • \$\begingroup\$ I have cleaned my code a bit thanks to you but will explain better. Or maybe for next time theres a way to upload the jframe itself? So, The radio buttons are the chosen values (1,5) and the buttons are the + and -. \$\endgroup\$ Nov 23, 2017 at 13:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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