refactoring legacy code for java 8

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:

  1. 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
  2. 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);
        }    
    }

eclipse neon (4.6) for the mac

eclipse.org went with a neon colored theme to announce the launch of Neon. I found it a bit glaring. The “e” and “n” lights go out after a while which I suppose is cute.  The matrix comparing the packages is still clear. It turns out not to matter if you choose the Java EE version or something else for the download. The list of Eclipse packages had a sponsored package in the list. Wonder how much IBM paid to have Bluemix listed second. I also learned there is a Scout package. I hadn’t heard of Scout which is a framework for HTML 5 among other things.

Overall, there’s a lot I’m excited about in this release.

The “tar” file (native app)

With Eclipse Mars, they switched to a tar file/Eclipse installer for Mac. This is my first upgrade since that Eclipse became a native Mac app. The installer says “Eclipse installer by Oomph” and gives you a choice of a number of Eclipse packages. Which means it doesn’t matter what you choose because it takes you to this point.

neon-installer

Then it asks where you want to install. This is good as it lets you have both Mars and Neon installed as native apps. (I was wondering how they were going to deal with that when Mars went native.)
neon-installer-dir

The default location seemed like as good a place as any. I clicked install and agreed to do the terms. As I saw the progress bar, I got prompted to agree again. As it was downloading the necessary pieces, I got a warning that downloading was slow.
neon-installer-slow

Then it was done and I was able to launch Eclipse. I got prompted for a workspace location. I like to upload my workspace in place so I agreed that I would be preventing the workspace from opening in Mars again. (I backed it up first in case.) Then I saw the Neon slash screen. I was a little worried about this since I didn’t like the home page. No reason to worry. It’s pretty!

neon-splash

Installing the plugins

Like last year, I decided to install the plugins I need for Eclipse Marketplace so I can shed the plugins I tried out and don’t actually want. Cleaning plugin house once a year is nice.

The significant plugins I use are listed in this table. A number of plugins were beta for Luna or I had to use the Kepler version. I don’t remember that problem in previous years.

Last year, I tried out the Code Recommenders plugin. I didn’t install it this year as I hardly used it. I added Contrast and Bytecode Analyzer as plugins I installed in the past 12 months that I like. Everything installed easily from Eclipse Marketplace unless otherwise noted.

Plugin Purpose
Mongrel Tomcat integration supporting recent versions of Tomcat.
Ecl Emma Code coverage
SonarLint  I installed SonarLint last year and quickly came to rely on it. It gives you static analysis findings in Eclipse. I also included the SonarLint Java Configuration Helper so it can see the version of Java I am using. (I”m on Java 8 right now so this is redundant at the moment. But I’m ready for when Java 9 comes out.) I stopped installing PMD and FindBugs. I’m using SonarLint instead.
Subversive To access Subversion repositories
Eclipse Memory Analyzer For finding memory leaks. It was in Eclipse MarketPlace – however I couldn’t install from there. It just kept prompting me to install. So I’m using the update site.
Freemarker IDE Freemarker syntax highlighting and macro assistance.  Note that it is listed under the JBoss Tool Project. You pick that plugin and then unselect everything except “Freemarker IDE”. The JBoss Tool plugin was in beta on Mars release day. I installed this beta.
Pydev Python plugin/perspective
Contrast To spot potential security issues. See my impressions of the Contrast plugin.
Bytecode Outline I’ve been looking at bytecode a good fit for the book to make sure I understand why things are happening. This plugin makes it easy. I first tried Bytecode Visualizer but install failed. (The website says there were 25 failed installs with the same dependency problem in the last 7 days). After installing Bytecode Outline, I realized this was the one I had installed for Luna anyway.

What excites me

  1. Autocomplete lets you enter any part of the class name/method name/variable/etc rather than just the first part. Being able to enter a substring for the pattern is awesome! If you know the method name ends with “all” you can type this. You can also type something that is more unique if you have a lot of classes that begin with the same thing. For example, suppose you have MyBusinessWidgetStrategy, MyBusinessWidgetDto and MyBusinessWidgetDao. You can type “widgetDao” and be done rather than the whole thing.
  2. You can use a touchpad to pinch/zoom in and out for the editor. This is going to be great for demos.
  3. The workspace name is shown at the beginning of the window title. This isn’t useful to me at all at home, but is going to be very useful at work where I frequently have multiple workspaces open at the same time. The default name of the workspace is the directory it is in. That actually works out perfectly for me
  4. Being able to easily clean up pre-diamond operator (Java 1.5 and 1.6 code) to get rid of the redundant types. (Wrote up how here.)
  5. You can control word wrap in Java and other text editors. While you typically want to format in Java, this could still be useful for viewing legacy code you don’t want to re-format.) toggle

What I didn’t like

  1. Mongrel didn’t work for launching Tomcat.
  2. I was hoping for code cleanup items for Java 8. In particular with regard to lambdas and streams. I didn’t see anything.

Other interesting features

  1. HTML formatting finally works the way I’d expect. I last complained about this in Juno so it might have been fixed for a while and I just never tried it again.
  2. You can set Preferences > General >Editors > Autosave to save your editor for you. I don’t like this because I want to control when I save since this sometimes triggers builds and such. I think it is nice that it is an option though.
  3. You can automatically terminate the previous run of a JUnit test (or other launcher) when you relaunch it. While I don’t need this anymore, it would have been useful when I was learning about recursion!
  4. It’s a good number of clicks to find a specific compile error/warning in the Eclipse preferences. You can now get there directly when you have something show up. There’s also another “info” level so you don’t have to choose between “warning” and “ignore.”
  5. You can now search in binary files. (I thought we could always do this, but I must be mistaken since it was in the release notes.)

eclipse neon – removing redundant types in java 7 diamond operators

I’m really excited that Neon comes with a feature to remove the redundant types that haven’t been needed since Java 7. For example, List<String> list = new ArrayList<String>(); vs List<String> list = new ArrayList<>(); We have the diamond operator, but there hasn’t been any easy way that I know of for dealing with the old code.

How to use it

I wanted to run JUST this rule so my commit doesn’t have other things in it. (I like to run these rules one at a time). Here’s what I did:

  1. Select project
  2. Source > Cleanup
  3. Use custom profile
  4. Configure
  5. Uncheck box on member access tab
  6. Uncheck box on missing code tab
  7. Uncheck all boxes on unnecessary code tab
  8. Check box for Remove redundant type arguments (1.7 or higher)
  9. Ok
  10. Finish
  11. Wait about 30 seconds
  12. Sync with repository
  13. Spot check three classes – all good!
  14. Commit the 202 files that had changes in it
  15. Enjoy the cleaner code 🙂