9
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ BTW, I think the second method is wrong. If the second new joiner is the last item in the collection, the method will incorrectly return false. \$\endgroup\$
    – svick
    Commented May 26, 2013 at 22:04
  • \$\begingroup\$ Strictly speaking, candidate.getRole().equals("Leader"); should be candidate.isRole("Leader"). \$\endgroup\$ Commented Jun 25, 2013 at 5:11
  • \$\begingroup\$ (Check what happens when amountOfNewJoiners gets incremented when member is the last item in team.) \$\endgroup\$
    – greybeard
    Commented Dec 26, 2019 at 9:47
  • \$\begingroup\$ A very nice example of why first class functions are good :) .filter() is all that would be needed here. \$\endgroup\$
    – JollyJoker
    Commented Dec 27, 2019 at 17:04

7 Answers 7

5
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ a nitpick: private static enum - now this is a redundancy, enum is already static by itself, just like an interface \$\endgroup\$ Commented Oct 15, 2015 at 13:40
4
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the answer. I will give it a try. By the way, I moved the question to this forum, maybe is more apropiate. \$\endgroup\$
    – sfrj
    Commented May 26, 2013 at 21:42
  • \$\begingroup\$ The false negative occurs whenever the second new joiner is the last member. Best to check conditions like that at the point where you increment the counter. \$\endgroup\$ Commented May 27, 2013 at 4:27
3
\$\begingroup\$

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;
\$\endgroup\$
1
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Yes, I'm very much looking forward to the appearance of real closures in Java 8! \$\endgroup\$ Commented May 27, 2013 at 4:30
  • \$\begingroup\$ @svick for large collections, performance considerations could be significant. I'd recommend team.Where(member => member.StartingData > aMonthAgo).Take(2).Count() == 2 instead. It would stop iterating over the collection after finding first two new joiners, which is perfectly sufficient. \$\endgroup\$ Commented Oct 15, 2015 at 13:51
  • \$\begingroup\$ @KonradMorawski Yeah, if you know performance is important, that would be an improvement. \$\endgroup\$
    – svick
    Commented Oct 15, 2015 at 14:29
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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);
    }

}
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.