Toggling between radio buttons

Question

I have several buttons and I want to toggle between them like radio buttons.

The way I have it set up now is that each button have this same code but the buttonValue varies depending on which button they click on.

ToggleButton one;
one = (ToggleButton) view.findViewById(R.id.number_one);
one.setOnClickListener(new View.OnClickListener() {
 public void onClick(View view) {
 buttonValue = 1;
 ToggleButton tb = (ToggleButton) view;
 if(tb.isChecked()) {
 RadioGroup gridLayout = (RadioGroup) tb.getParent();
 for(int i=0; i<(gridLayout).getChildCount(); ++i) {
 View nextChild = (gridLayout).getChildAt(i);
 if(nextChild instanceof ToggleButton && nextChild.getId()==tb.getId()){
 }else if (nextChild instanceof ToggleButton && nextChild.getId()!=tb.getId() ){
 ToggleButton tb2=(ToggleButton) nextChild;
 tb2.setChecked(false);
 }
 }
 } else{
 RadioGroup gridLayout = (RadioGroup) tb.getParent();
 for(int i=0; i<(gridLayout).getChildCount(); ++i) {
 View nextChild = (gridLayout).getChildAt(i);
 if(nextChild instanceof ToggleButton && nextChild.getId()==tb.getId() ){
 ToggleButton tb2=(ToggleButton) nextChild;
 tb2.setChecked(false);
 }else if (nextChild instanceof ToggleButton && nextChild.getId()!=tb.getId() ){
 ToggleButton tb2=(ToggleButton) nextChild;
 tb2.setChecked(false);
 }
 }
 }
 }
});

I basically have the same code for 10 buttons! While the code works and the buttons toggle between each other, I don't think is the best approach (especially because it's so many lines of code for 10 buttons where the main isChecked() function is exactly the same in every onClick button listener.

I was wondering if there was a way so that I only have one button listener that increments the buttonValue based on what button was clicked and keeps the main isChecked() function the same.

Answer 1

The trick here is to create an inner class.

Something I've started doing recently is to create a method that returns the interface you want!

private View.OnClickListener createClickListener(final int value) {
 return new View.OnClickListener() {
 buttonValue = value;
 // the rest of your code goes here
 };
}

Thanks to marking the int value as final in the method declaration, it is accessible also from the inner class.

You can now use this like:

one.setOnClickListener(createClickListener(1));

---

Your actual code

While reducing the code duplication for the individual buttons is important, let's take a look at the code that is actually performed for each button!

Step 1. Adjust spacing to adhere to the Java coding standards and remove unnecessary parenthesis.

Step 2. gridLayout is used in both if and else, so put it outside.

Step 3. In the else part, you're doing the same thing no matter if nextChild.getId() and tb.getId() is the same or not, so reduce code duplication by simplifying that logic.

Step 4. Remove empty if statement

After these steps, the code is:

ToggleButton tb = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) tb.getParent();
if (tb.isChecked()) {
 for (int i = 0; i < gridLayout.getChildCount(); ++i) {
 View nextChild = (gridLayout).getChildAt(i);
 if (nextChild instanceof ToggleButton && nextChild.getId() != tb.getId()) {
 ToggleButton tb2 = (ToggleButton) nextChild;
 tb2.setChecked(false);
 }
 }
} else {
 for (int i = 0; i < gridLayout.getChildCount(); ++i) {
 View nextChild = gridLayout.getChildAt(i);
 if (nextChild instanceof ToggleButton) {
 ToggleButton tb2 = (ToggleButton) nextChild;
 tb2.setChecked(false);
 }
 }
}

Next steps:

• Put the for loop outside the if-statement
• continue early if it is not an instance of ToggleButton
• Adjust the if condition to take into consideration the tb.isChecked() from outside, and use an || (OR) operation for the logic.
• Rename tb to clickedButton which more describes what the button is used for.

I believe this code will have the exact same effect as your original:

ToggleButton clickedButton = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) clickedButton.getParent();
for (int i = 0; i < gridLayout.getChildCount(); ++i) {
 View nextChild = gridLayout.getChildAt(i);
 if (!(nextChild instanceof ToggleButton)) {
 continue;
 }
 if (nextChild.getId() != clickedButton.getId() || !clickedButton.isChecked()) {
 ToggleButton tb2 = (ToggleButton) nextChild;
 tb2.setChecked(false);
 }
}

Answer 2

Another alternative to @Simon's suggestion is using a static member class, something like this:

private static class CustomButtonClickListener implements OnClickListener {
 private final int buttonValue;
 public CustomButtonClickListener(int buttonValue) {
 this.buttonValue = buttonValue;
 }
 @Override
 public void onClick(View view) {
 // ...
 }
}

Then you could add click listeners to your buttons like this:

one.setOnClickListener(new CustomButtonClickListener(1));
two.setOnClickListener(new CustomButtonClickListener(2));
// ... and so on

Actually, probably it will be better to put your buttons in an array and use a loop to add the listeners with the correct buttonValue parameters, to avoid the possibility of typos.

But which way should you go? A factory method like Simon's, or a static member class? The static member class is more extensible, as you give it new fields, like buttonValue in the above example. If the posted code is all you ever want to do with your click listener, then it doesn't really matter, and whichever solution will work just fine.

Another thing, after Simon's nice refactoring, it's easier to see that the logic of onClick becomes this:

if tb.isChecked():
 setChecked(false) for all other buttons
else:
 setChecked(false) for all buttons

... which, seems a bit silly, doesn't it? If the button is not checked, why reset all the buttons? Why not just leave them alone? It seems to me you could simply drop the else block, so the code becomes simply:

@Override
public void onClick(View view) {
 ToggleButton tb = (ToggleButton) view;
 RadioGroup radioGroup = (RadioGroup) tb.getParent();
 if (tb.isChecked()) {
 return;
 }
 for (int i = 0; i < radioGroup.getChildCount(); ++i) {
 View nextChild = radioGroup.getChildAt(i);
 if (nextChild instanceof ToggleButton && nextChild.getId() != tb.getId()) {
 ((ToggleButton) nextChild).setChecked(false);
 }
 }
}

I also renamed the gridLayout variable to radioGroup, to better reflect what it is.

• Simon Forsberg: I was also considering going with the private static class approach first, thanks for covering that for me. The buttonValue in the original code is, as far as I can tell, not a field on the inner class though but on the outer one, so the class cannot be static to be able to change that.
• Kala J: Thanks for pointing out the radioGroup/gridLayout confusion! That has been changed :D