Return to Answer

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.

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.

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 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.