2
\$\begingroup\$

I have two these methods. Their loops are pretty similar. I'd like to refactor it to avoid duplicate nested loops. How could I do that?

@Override
public boolean isValid() {
    for (final FormSection formSection : getFormSections()) {
        for (final Component fieldComponent : formSection) {
            boolean isFieldValid = ((Field)fieldComponent).isValid();
            if(!isFieldValid){
                return false;
            }
        }
    }
    return true;
}

private int countErrors() {
    int errorAmount = 0;
    for (final FormSection formSection : getFormSections()) {
        for (final Component fieldComponent : formSection) {
            boolean isFieldValid = ((Field)fieldComponent).isValid();
            if(!isFieldValid){
                errorAmount++;
            }
        }
    }
    return errorAmount;
}

I've tried to use interface but it doesn't look good.

interface FieldHandler {
    boolean process(AtomicInteger number);
}

@Override
public boolean isValid() {
    return handleTextFieldsInTabs(new FieldHandler() {
        @Override
        public boolean process(AtomicInteger number) {
            return false;
        }
    }, null);
}

boolean handleTextFieldsInTabs(FieldHandler fieldHandler, AtomicInteger number) {
    for (final FormSection formSection : getFormSections()) {
        for (final Component fieldComponent : formSection) {
            boolean isFieldValid = ((Field)fieldComponent).isValid();
            if(!isFieldValid && !fieldHandler.process(number)) {
                return false;
            }
        }
    }
    return true;
}

private int countErrors() {
    final AtomicInteger errorAmount = new AtomicInteger(0);
    handleTextFieldsInTabs(new FieldHandler() {
        @Override
        public boolean process(AtomicInteger number) {
            number.incrementAndGet();
            return true;
        }
    }, errorAmount);
    return errorAmount.intValue();
}
\$\endgroup\$
  • \$\begingroup\$ Are you on Java 8? \$\endgroup\$ – h.j.k. Apr 4 '16 at 4:00
  • 1
    \$\begingroup\$ Users can use Java 7 to run the code. That's why I need a non-lambda way. \$\endgroup\$ – emeraldhieu Apr 4 '16 at 4:06
  • \$\begingroup\$ Do you have any 3rd party library like Apache's common-collections4 or Google's guava? \$\endgroup\$ – Genzer Apr 4 '16 at 4:14
1
\$\begingroup\$

If you are using Google's guava, I would suggest:

@Override
public boolean isValid() {
  // Firstly, flat your Iterable<Iterable<Component>> getFormSections()
  // into Iterable<Component>.
  // Then we use a Predicate to check whether there is any match.
  return Iterables.all(
    Iterables.concat(getFormSections()), validComponents()); 
}

@Override
public int countErrors() {
   return Iterables.size(
      Iterables.filter(Iterables.concat(getFormSections()), Predicates.not(validComponents()));

}

// - Positive names is always preferable
// - This method is static simply because it does not access to any instance varibles. It is easier to extract it out into another class.
private static Predicate<Component> validComponent() {
  new Predicate<Component>() {
      @Override
      public boolean apply(Component input) {
        return // your logic here
      }
    };
}
\$\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.