13
\$\begingroup\$

Instead of writing four almost identical setOnClickListener method calls, I opted to iterate over an array, but I'm wondering if this was the best way to do it. Do you have any suggestions for improving it?

/* Set up the radio button click listeners so two categories are not selected
   at the same time. When one of them is clicked it clears the others.
*/
final RadioButton[] buttons = {radio_books,radio_games,radio_dvds,radio_electronics};
for (int i = 0; i < 4; i++) {
    final int k = i;
    buttons[i].setOnClickListener(new OnClickListener() {
        @Override
        public void onClick(View v) {
            for (int j = 0; j < 4; j++) {
                if (buttons[j] != buttons[k]) {
                    buttons[j].setChecked(false);
                }
            }
        }
    });
}
\$\endgroup\$
  • \$\begingroup\$ One thing I'd recommend is to use lowerCamelCase for variable names. That's the variable naming convention for Java \$\endgroup\$ – stratwine Sep 2 '11 at 18:24
6
\$\begingroup\$

What GUI framework is this? Is the View v being passed to the OnClick method actually the clicked RadioButton? If so, here are some changes to consider

final RadioButton[] buttons = {radio_books,radio_games,radio_dvds,radio_electronics};

final OnClickListener onClickHandler =
    new OnClickListener() {
        @Override
        public void onClick(final View v) {
            final RadioButton checkedButton = (RadioButton) v;
            for (final RadioButton button : buttons) {
                if (button != checkedButton) {
                    button.setChecked(false);
                }
            }
        }
    };

for (final RadioButton button : buttons) {
    button.setOnClickListener(onClickHandler);
}

If View v != the checked radio button, then:

final RadioButton[] buttons = {radio_books,radio_games,radio_dvds,radio_electronics};
for (final RadioButton thisButton: buttons) {
    thisButton.setOnClickListener(
        new OnClickListener() {
            @Override
            public void onClick(final View v) {
                for (final RadioButton button : buttons) {
                    if (button != thisButton) {
                        button.setChecked(false);
                    }
                }
            }
        }
    );
}
\$\endgroup\$
  • \$\begingroup\$ Why are you still looping over the button array in the listener in your first solution? \$\endgroup\$ – Paul Whitehurst Feb 6 '11 at 6:17
  • \$\begingroup\$ @Paul - Because he's trying to uncheck the buttons that were not the clicked one, so it loops to get to all the buttons and uses the if to exclude the one that was clicked. Or am I misunderstanding your question? \$\endgroup\$ – Bert F Feb 6 '11 at 6:20
  • \$\begingroup\$ @Paul - lol - its 1:24am here - I risking misread the code myself, so I understand the feeling. Actually, thanks for reminding me of the time :-) need sleep ... \$\endgroup\$ – Bert F Feb 6 '11 at 6:26
  • 1
    \$\begingroup\$ It's the android GUI framework. The RadioGroup that handles the boilerplate of making sure only one radio button is active at a time failed on me because I need a layout that was a bit more complicated. \$\endgroup\$ – davidk01 Feb 6 '11 at 7:09
  • 1
    \$\begingroup\$ Ok, thanks for the feedback. I think with the improved 'for' loops and using the View argument of onClick to perform the exclusion it can't be improved further. \$\endgroup\$ – davidk01 Feb 6 '11 at 7:20
2
\$\begingroup\$

It's typical callback method, you just defined setOnClickListener once. Calling 4 times is not bad smell.

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