Computing predicates of multiple items avoiding duplication of iteration code

Question

I am breaking my head with how to get rid of semantic duplication(Code that is syntactically the same but does different things).

I can't find anywhere a post or something that mentions a bit how to refactor this kind of duplication. All I found was this:
Duplicate Code and Ceremony in Java but it does not go into detail on how to refactor it.

This is the code that is causing me problem:

public class TeamValidator {       

      public boolean isThereALeader(List<Member> team) {  
           Iterator<Member> iterator = team.iterator();
           while(iterator.hasNext()) {
              Member member = iterator.next();
              String role = member.getRole();
              if(role.equals("Leader"))
                return true;
           }  
           return false;
      }  

      public boolean areThereAtLeast2NewJoiners(List<Member> team) { 
            int amountOfNewJoiners = 0; 
            for(Member member:team) {
                if(amountOfNewJoiners == 2)
                    return true;
                DateTime aMonthAgo = DateTime.now().minusMonths(1);
                if(member.startingDate().isAfter(aMonthAgo)) {
                   amountOfNewJoiners++;
                }
            }
            return false;   
      }
}

In these two methods there is semantic duplication, because both iterate a list and also check some condition/s. Any idea how could I make this semantic duplication disappear?
I would really appreciate some tip or suggestion on how to refactor this.

Answer 1

Key here is to identify that both functions check whether there are at least n members that match a certain condition.

So we introduce a Matcher interface :

public interface Matcher<T> {

    boolean matches(T candidate);

}

Which we then use to make a method hasAtLeastNMatches(). And both methods can be implemented by calling that.

public class TeamValidator {

    public boolean isThereALeader(List<Member> team) {
        return hasAtLeastNMatches(team, Is.Leader, 1);
    }

    public boolean areThereAtLeast2NewJoiners(List<Member> team) {
        final DateTime aMonthAgo = DateTime.now().minusMonths(1);
        return hasAtLeastNMatches(team, Is.NewJoiner, 1);
    }

    private boolean hasAtLeastNMatches(Iterable<Member> members, Matcher<Member> condition, int minimumNumberOfMatches) {
        int count = 0;
        for (Member member : members) {
            if (condition.matches(member) && ++count >= minimumNumberOfMatches) {
                return true;
            }
        }
        return false;
    }

    private static enum Is implements Matcher<Member> {
        Leader {
            @Override
            public boolean matches(Member candidate) {
                return candidate.getRole().equals("Leader");
            }
        },

        NewJoiner {
            @Override
            public boolean matches(Member candidate) {
                return candidate.startingDate().isAfter(DateTime.now().minusMonths(1));
            }
        }
    }

}

Method hasAtLeastNMatches() can obviously also be reused ouside this class. In fact this kind of reuse is so common that several open source code projects offer these kinds methods and interfaces: e.g. Apache commons collections and Guava . In fact what I called Matcher they both call Predicate.

Answer 2

Well for a start, choose a style: Use the enhanced for loop, or use explicit iterators. Don't use one in one place and another in another. Other than that, I'm not seeing enough similar in the loops to justify spending any time on it as opposed to, say, actually getting something else done. :-)

But if you really, really want to, you can create an interface that evaluates loop conditions, implement concrete classes for the two situations, and use them. I don't think it buys you much.

(The below also fixes the false negative if you have exactly two members and they're both new joiners.)

Definition:

abstract class LoopEvaluator<T> {
    public boolean eval(List<T> list) {
        for (T element : list) {
            if (this.evalElement(element)) {
                return true;
            }
        }
        return false;
    }
    abstract boolean evalElement(T element);
}

class LookForLeaders<T extends Member> extends LoopEvaluator<T> {
    @override
    public bool evalElement(T element) {
        return element.getRole().equals("Leader");
    }
}

class LookForTwoNewJoiners extends LoopEvaluator<T> {
    private int newJoiners;

    @override
    public boolean eval(List<T> list) {

        this.newJoiners = 0;

        return super.eval(list);
    }

    @override
    public boolean evalElement(T element) {
        DateTime aMonthAgo = DateTime.now().minusMonths(1);
        if(member.startingDate().isAfter(aMonthAgo)) {
           this.newJoiners++;
        }
        return newJoiners == 2;
    }
}

Use:

public class TeamValidator {       

    @override
    public boolean isThereALeader(List<Member> team) {  
        return new LookForLeader().eval(team);
    }  

    @override
    public boolean areThereAtLeast2NewJoiners(List<Member> team) { 
        return new LookForTwoNewJoiners().eval(team);
    }
}

It's late and I haven't double-checked the syntax, but you get the idea...

Answer 3

I would use FluentIterable from Google's Guava libraries to do away with a lot of boiler-plate, consider the following:

public class TeamValidator {       

    public boolean isThereALeader(List<Member> team) {  
        return FluentIterable.from(team).anyMatch(new Predicate<Member>() {
            @Override
            public boolean apply(Member member) {
                return member.getRole().equals("Leader");
            }
        });
    }  

    public boolean areThereAtLeast2NewJoiners(List<Member> team) { 
        final DateTime aMonthAgo = DateTime.now().minusMonths(1);
        return FluentIterable.from(team).filter(new Predicate<Member>() {
            @Override
            public boolean apply(Member member) {
                return member.startingDate().isAfter(aMonthAgo);
            }
        }).size() >= 2;           
    }
}

This can further be improved by extracting the Predicates as classes, e.g.:

...
public boolean isThereALeader(List<Member> team) {  
    return FluentIterable.from(team).anyMatch(new TeamLeaderExistsPredicate());
}
...

This allows the logic contained in the Predicate to be reused, if needs be, and is easier on the eye.

EDIT:

Java 8 equivalent:

boolean leaderExists = team.stream().anyMatch(m -> "Leader".equals(m.getRole()));

LocalDateTime aMonthAgo = LocalDateTime.now().minusMonths(1);
boolean atLeastTwoNewJoinersExist = team.stream()
    .filter(m -> m.startingDate().isAfter(aMonthAgo))
    .count() >= 2;

Answer 4

This is a case where lambdas and a library of higher-order functions are very useful. For example in C#, both of your methods are pretty much one-liners:

public bool IsThereALeader(List<Member> team)
{
    return team.Any(member => member.Role == "Leader");
}

public bool AreThereAtLeast2NewJoiners(List<Member> team)
{
    DateTime aMonthAgo = DateTime.Now.AddMonths(-1);
    return team.Count(member => member.StartingDate > aMonthAgo) >= 2;
}

(The second method will iterate the whole collection even if it finds two new joiners early on, but that's not likely to matter.)

In Java, you can emulate lambdas using anonymous classes, but the syntax is much more verbose (and I don't know if there is a library that provides the required helper functions).

There are also some libraries listed at the Stack Overflow question What is the Java equivalent for LINQ? that you could use for this purpose.

Answer 5

both Iterate a list and also check some condition/s and then return true if the condition is met for any element in the collection, otherwise they return false.

It's exactly what you describe, lets write the code:

We have a list or collection to make it more generic. List<T>.

Then we have some conditions checked against T elements. You can define an interface for a condition.

interface Condition<T> {
    boolean check (t t);
}

You can create a new condition for whatever you want to check.

Now all you need is a method that iterates a collection of Ts and checks them all against a condition. Put that in a utility class and you're done>

static boolean checkAgainstCondition(Collection<T> collection, Condition<T> condition) {
    for (T t: collection) {
        if (condition.check(t) {
            return true;
        }
    }
    return false;
 }

Similarly you can have utility methods like:

static int countMatches(Collection<T> collection, Condition<T> condition) {
    //count how many elements meet the condition.

or

static boolean moreMatchesThan(Collection<T> collection, Condition<T> condition, count) {
    //return true if there are more than count elements in the collection that meet the condition.

You can do the same trick also in similar cases for example when you want to select an element from the collection that matches the condition, or if you want to remove from the collection elements that match to a condition etc. It's just the condition interface + utility class.

You can see the idea implemented in this utility project. Condition interface, utility class 1 and utility class 2.

Answer 6

You can use the functional style for this task. This will help splitting the logic and general loop, so it simplifies the reading the code.

I've wrote this in sublime, so there may be some errors in code, but the main idea is clear from this example:

public class TeamValidator {

    public static interface Function <R, T> {
        R apply(T val);
    }    

    public boolean generalIteration(Function<Boolean, Member> func, List<Member> list) {
        Boolean result = false;
        for (Member member : list) {
            result = func.apply(member);
            if (result) break;
        }
        return result;
    }

    public boolean isThereALeader(List<Member> team) {
        return generalIteration(new Function<Boolean, Member>() {
            @Override
            public Boolean apply(Member member) {
                return "Leader".equals(member.getRole());
            }
        }, team);
    }

    public boolean areThereAtLeast2NewJoiners(List<Member> team) {
        final int[] amountOfNewJoiners = {0};
        return generalIteration(new Function<Boolean, Member>() {
            @Override
            public Boolean apply(Member member) {
                DateTime aMonthAgo = DateTime.now().minusMonths(1);
                if (member.startingDate().isAfter(aMonthAgo)) {
                    int val = amountOfNewJoiners[0];
                    amountOfNewJoiners[0] = ++val;
                }
                return (amountOfNewJoiners[0] == 2);
            }
        }, team);
    }

}

Answer 7

If code is syntactically the same, it is the same code... I think you are over-thinking this. The two code examples above are different enough to warrant their existence.

Now, if you wanted to eliminate two methods that did the exact same thing, but lived in two places, that is simple. Just delete the one with less usage, then replace all calls to the old method with the new one. This is incredibly simple if they share the same method signature, slightly more involved if they do not.