Skip to main content
Code Review

Return to Answer

Commonmark migration
Source Link

#Your actual code

Your actual code

#Your actual code

Your actual code

added 128 characters in body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
  • 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.
ToggleButton tbclickedButton = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) tbclickedButton.getParent();
for (int i = 0; i < gridLayout.getChildCount(); ++i) {
 View nextChild = gridLayout.getChildAt(i);
 if (!(nextChild instanceof ToggleButton)) {
 continue;
 }
 if (nextChild.getId() != tbclickedButton.getId() || !tbclickedButton.isChecked()) {
 ToggleButton tb2 = (ToggleButton) nextChild;
 tb2.setChecked(false);
 }
}
  • 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.
ToggleButton tb = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) tb.getParent();
for (int i = 0; i < gridLayout.getChildCount(); ++i) {
 View nextChild = gridLayout.getChildAt(i);
 if (!(nextChild instanceof ToggleButton)) {
 continue;
 }
 if (nextChild.getId() != tb.getId() || !tb.isChecked()) {
 ToggleButton tb2 = (ToggleButton) nextChild;
 tb2.setChecked(false);
 }
}
  • 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.
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);
 }
}
added 2230 characters in body
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311

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

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

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

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

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

ToggleButton tb = (ToggleButton) view;
RadioGroup gridLayout = (RadioGroup) tb.getParent();
for (int i = 0; i < gridLayout.getChildCount(); ++i) {
 View nextChild = gridLayout.getChildAt(i);
 if (!(nextChild instanceof ToggleButton)) {
 continue;
 }
 if (nextChild.getId() != tb.getId() || !tb.isChecked()) {
 ToggleButton tb2 = (ToggleButton) nextChild;
 tb2.setChecked(false);
 }
}
Source Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
Loading
lang-java

AltStyle によって変換されたページ (->オリジナル) /