I asked a teammate at CodeRanch if I could use a method he wrote to show how it could be written more clearly/succinctly in Java 8. He said yes. This is the original method. It is 95 lines and has a cyclomatic complexity of 27.
public static Map<Category, List<Forum>> getBetaTopForums(BestTopicsDAO bestTopicDAO, int userId, List<Topic> bestThisYear, List<Topic> secondaryBestList){
Map<Integer, Integer> ratings = bestTopicDAO.selectForumRatingsByUserId(userId);
List<Integer> forumIds = new ArrayList<>();
List<Integer> ratedForumIds = CollectionCommon.getKeysSortedByValues(ratings, true);
final int defaultUserId = SystemGlobals.getIntValue(ConfigKeys.BEST_TOPICS_DEFAULT_USER_ID);
// Let's first add the forums that are rated above 5/10
for(Integer forumId: ratedForumIds){
Integer r = ratings.get(forumId);
if(r != null && r > 5){
forumIds.add(forumId);
}
}
// Now let's add the forums found in best topics.
if(bestThisYear != null){
for(Topic topic: bestThisYear){
int forumId = topic.getForumId();
if(!forumIds.contains(forumId)){
forumIds.add(forumId);
}
}
}
if(secondaryBestList != null){
for(Topic topic: secondaryBestList){
int forumId = topic.getForumId();
if(!forumIds.contains(forumId)){
forumIds.add(forumId);
}
}
}
// If this user is not the default-rating-user, let's add the top forums
// rated by the default-rating-user.
if(userId != defaultUserId) {
Collection<Integer> fids = getTopRatedForumIds(bestTopicDAO);
for(Integer fid: fids) {
if(fid != null && !forumIds.contains(fid)) {
forumIds.add(fid);
}
}
}
// Let's also add some forums found in hot topics.
List<Topic> tmpTopics = TopicRepository.getHottestTopics();
for(Topic topic: tmpTopics){
int forumId = topic.getForumId();
if(!forumIds.contains(forumId)){
forumIds.add(forumId);
}
}
// Finally, let's add the forums that were rated below 6/10
for(Integer forumId: ratedForumIds){
Integer r = ratings.get(forumId);
if(r != null && r < 6 && !forumIds.contains(forumId)){
forumIds.add(forumId);
}
}
Map<Category, List<Forum>> forums = new LinkedHashMap<>();
PermissionControl pc = SecurityRepository.get(userId);
final int maxTopForums = SystemGlobals.getIntValue(ConfigKeys.BETA_VIEW_MAX_TOP_FORUMS_IN_HOME);
int forumsToShow = 0;
// We need to remove the forums that are not visible, or rated 0/10
for(Integer forumId: forumIds){
if(pc.canAccess(SecurityConstants.PERM_FORUM, String.valueOf(forumId))){
Forum f = ForumRepository.getForum(forumId);
Category c = f != null ? ForumRepository.getCategory(f.getCategoryId()) : null;
if(f != null && c != null && f.isShowInForumList()){
if(++forumsToShow > maxTopForums){
break;
}
List<Forum> fs;
if(forums.containsKey(c)){
fs = forums.get(c);
}else{
fs = new ArrayList<>();
forums.put(c, fs);
}
fs.add(f);
}
}
}
return forums;
}
First step – write unit tests
First, I wrote unit tests for the existing code to ensure I didn’t break anything. While doing that, I did some minor refactorings. (I didn’t do extract method to make the method shorter since I knew I’d be updating that later). What I did change:
- Removed BestTopicsDAO as a parameter. It comes from a factory and we have a mock framework setup for that factory already. So there was no reason it had to be a parameter
- Renamed bestThisYear to primaryBestList. (It doesn’t always represent the best for the year)
I couldn’t get 100% test coverage because a few of the null checks were unreachable due to early logic (in the helper methods.) I left them in to see the effect they’d have on conversion.
Refactoring to Java 8 – Getting started
There’s a lot of for loops here. And I suspect they are very similar. Let’s start with the first one:
for(Integer forumId: ratedForumIds){
Integer r = ratings.get(forumId);
if(r != null && r > 5){
forumIds.add(forumId);
}
}
I converted this to a helper method and a stream:
forumIds.addAll(ratedForumIds.stream().filter(id -> {
Integer r = ratings.get(id);
return r != null && r > 5;
}).collect(Collectors.toList()));
I’ll grant you that this code isn’t shorter than the original code. It is more similar in form to the code I expect to write next though. Also, it’s not easier to read given I didn’t extract the two line lambda. I’m going to do that after I get rid of the similar for loops.
The next refactoring
The next one is interesting. It’s just a for loop and if statement which is easy to rewrite.
for(Topic topic: primaryBestList){
int forumId = topic.getForumId();
if(!forumIds.contains(forumId)){
forumIds.add(forumId);
}
}
which is equivalent to
forumIds.addAll(primaryBestList.stream() .map(Topic::getForumId) .filter(f -> ! forumIds.contains(f)) .collect(Collectors.toList()));
However, it is interesting because it check if the forum is already in forumIds. This shouldn’t be necessary as it is something a data structure could do. We need a data structure that is both a list (preserves order) and a set (checks for uniqueness.) Enter LinkedHashSet. It’s a set and preserves the insertion order. The JavaDoc even specifies that if you add the same element again, the order doesn’t change. Perfect. Switching forumIds to a LinkedHashSet allows for getting rid of the filter intermediate operation. And the unit tests still work. I used the same techniques for the next five for loops.
Time to use methods
I converted two methods that had to do with ratings and three that were straight conversions. I could have one common method they all use with identities for the filter/map that don’t apply. Two methods would be clearer. Giving me:
private static void addForumIds(Collection<Integer> forumIds, List<Topic> candidates) {
if (candidates != null) {
List<Integer> toAdd = candidates.stream().map(Topic::getForumId).collect(Collectors.toList());
forumIds.addAll(toAdd);
}
}
private static void addForumIds(Collection<Integer> forumIds, Collection<Integer> candidates, Predicate<Integer> predicate) {
if (candidates != null) {
List<Integer> toAdd = candidates.stream().filter(predicate).collect(Collectors.toList());
forumIds.addAll(toAdd);
}
}
The last one
The last chunk of code needed more refactoring to take care of all the forum filtering logic first. It also needed a mutable object for the counter rather than a primitive so it could be updated inside a lambda.
Conclusion
I could refactor this more. There’s duplication in the two lambdas. And I could extract more code into separate methods. Interestingly, the total code base is about the same as before. (It’s actually three lines longer, but I have more than three lines of comments.) But the method with the main logic is shorter and far less complicated:
public static Map<Category, List<Forum>> getBetaTopForums(int userId, List<Topic> primaryBestList, List<Topic> secondaryBestList){
BestTopicsDAO bestTopicDAO = DataAccessDriver.newBestTopicDAO();
Map<Integer, Integer> ratings = bestTopicDAO.selectForumRatingsByUserId(userId);
Collection<Integer> forumIds = new LinkedHashSet<>();
List<Integer> ratedForumIds = CollectionCommon.getKeysSortedByValues(ratings, true);
final int defaultUserId = SystemGlobals.getIntValue(ConfigKeys.BEST_TOPICS_DEFAULT_USER_ID);
// Let's first add the forums that are rated above 5/10
addForumIds(forumIds, ratedForumIds, id -> {
Integer r = ratings.get(id);
return r != null && r > 5;
});
// Now let's add the forums found in best topics.
addForumIds(forumIds, primaryBestList);
addForumIds(forumIds, secondaryBestList);
// If this user is not the default-rating-user, let's add the top forums
// rated by the default-rating-user.
if(userId != defaultUserId) {
Collection<Integer> fids = getTopRatedForumIds(bestTopicDAO);
addForumIds(forumIds, fids, id -> id != null);
}
// Let's also add some forums found in hot topics.
List<Topic> tmpTopics = TopicRepository.getHottestTopics();
addForumIds(forumIds, tmpTopics);
// Finally, let's add the forums that were rated below 6/10
addForumIds(forumIds, ratedForumIds, id -> {
Integer r = ratings.get(id);
return r != null && r < 6;
});
Map<Category, List<Forum>> forums = new LinkedHashMap<>();
PermissionControl pc = SecurityRepository.get(userId);
final int maxTopForums = SystemGlobals.getIntValue(ConfigKeys.BETA_VIEW_MAX_TOP_FORUMS_IN_HOME);
AtomicInteger countForumsToShow = new AtomicInteger(0);
// We need to remove the forums that are not visible, or rated 0/10
forumIds.stream()
// skip if can't access forum
.filter(id -> pc.canAccess(SecurityConstants.PERM_FORUM, String.valueOf(id)))
// get forum
.map(ForumRepository::getForum)
// skip if forum not found or can't access
.filter(f -> f != null && f.isShowInForumList())
// get category
.forEach(f -> addForumToCategory(forums, f, maxTopForums, countForumsToShow));
return forums;
}
private static void addForumToCategory(Map<Category, List<Forum>> forums, Forum f, int maxTopForums,
AtomicInteger countForumsToShow) {
Category c = ForumRepository.getCategory(f.getCategoryId());
if (c != null && countForumsToShow.incrementAndGet() <= maxTopForums) {
List<Forum> fs;
if (forums.containsKey(c)) {
fs = forums.get(c);
} else {
fs = new ArrayList<>();
forums.put(c, fs);
}
fs.add(f);
}
}
private static void addForumIds(Collection<Integer> forumIds, List<Topic> candidates) {
if (candidates != null) {
List<Integer> toAdd = candidates.stream().map(Topic::getForumId).collect(Collectors.toList());
forumIds.addAll(toAdd);
}
}
private static void addForumIds(Collection<Integer> forumIds, Collection<Integer> candidates, Predicate<Integer> predicate) {
if (candidates != null) {
List<Integer> toAdd = candidates.stream().filter(predicate).collect(Collectors.toList());
forumIds.addAll(toAdd);
}
}