Warning: this blog contains ugly code requiring horizontal scroll bars. Viewer discretion advised

Yesterday, we found the following badass query in our **CompletionStatusRepository** class:

    public List keysOfCompletedPagesFor(UserProfile user, String albumId, Language devLanguage) {
      String sql = "select pageStats.elementKey from PageCompletionStatus page " +
        "join AlbumStatistics albumStats on (albumStats.albumId = page.albumId and albumStats.devLanguage = page.devLanguage) " +
        "join TrackStatistics trackStats on (trackStats.albumStatistics_id = albumStats.id) " +
        "join PageStatistics pageStats on (pageStats.trackStatistics_id = trackStats.id and page.elementKey = pageStats.elementKey) " +
        "where page.user_id = :userId " +
        "and page.albumId = :albumId " +
        "and page.devLanguage = :devLanguage ";
      Session session = getSession();
      final SQLQuery query = session.createSQLQuery(sql);
      query.setLong"userId", user.getId());
      query.setString("albumId", albumId);
      query.setString("devLanguage", devLanguage.key);
      @SuppressWarnings("unchecked")
      List result = query.list();
      session.close();
      return result;
    }

We looked at the above code and said huh?

So many joins!

WTF?

Could it really be that hard to get this information?

WTF!!

No, there had to be an easier way.

My pair programmer that afternoon, Adam Sroka, and I puzzled our way into this logic and the code that calls it.

For a while, we just went in circles.

Then we got somewhere.

We sketched out some pseudo-code and agreed that it could work.

We already had good test coverage for this code, so we felt free to refactor.

We did not start by refactoring the badass query.

We began by working on the code that calls the query.

That code looked like this:

    public List uniqueIdsFor(ViewableAlbum viewableAlbum, UserProfile userProfile, CompletionStatusRepository repository) {
      List keys = repository.keysOfCompletedPagesFor(userProfile, viewableAlbum.getId(), viewableAlbum.getDevLanguageEnum());
    
      Listids = new ArrayList();
      for (String key : keys) {
        String path = getPagePathFor(key);
        Page page = viewableAlbum.getAlbum().getPage(path);
        ids.add(page.getUniqueId());    }    return ids;  
         }

The problem with the uniqueIdsFor(...) method is that it relied heavily on the badass query in the keysOfCompletedPagesFor(...) method.

We reasoned that ViewableAlbum already knew the information that the code was working so hard to re-acquire via the query’s joins.

To try out our experiment, we first test-drove a new method on the CompletionStatusRepository class:

    public boolean isPageCompleteFor(UserProfile user, String albumId, String pageKey, Language devLanguage) {
      return getPageCompletionFor(user, albumId, devLanguage, pageKey) != null;
    }

That new method reused the existing getPageCompletionFor(...) method, which itself uses simple HQL (Hibernate Query Language) based criteria methods:

    public PageCompletionStatus getPageCompletionFor(UserProfile user, String albumId, Language devLanguage, String pageKey) {
      List criteria = criteriaFor(user, albumId, devLanguage);
      criteria.add(Restrictions.eq("elementKey", pageKey));
      return (PageCompletionStatus) selectUnique(PageCompletionStatus.class, criteria);
    }
    
    private ListcriteriaFor(UserProfile user, String albumId, Language devLanguage) {
      List criteria = new ArrayList();
      criteria.add(Restrictions.eq("user", user));
      criteria.add(Restrictions.eq("albumId", albumId));
      criteria.add(Restrictions.eq("devLanguage", devLanguage.key));
      return criteria;
    }

Now we were ready to try our experiment. So we went back to the uniqueIdsFor(...) method and rewrote it to use a simple loop:

    public List uniqueIdsFor(ViewableAlbum viewableAlbum, UserProfile userProfile, CompletionStatusRepository repository) {
      Language albumLanguage = viewableAlbum.getDevLanguageEnum();
      List ids = new ArrayList();
      for (Page page : viewableAlbum.getAlbum().getPages())
        if (repository.isPageCompleteFor(userProfile, findSourceAlbumIdFor(page.getKey()), page.getKey(), albumLanguage))
          ids.add(page.getUniqueId());
      return ids;
    }

That worked!

Yes, it performs a query for every page, yet we reasoned that since our albums have less than 150 pages, the performance would be fine (and if not, we could optimize later).

This refactoring positioned us to make some deeper design changes that were needed to continue developing this new feature we’re working on.

It felt extremely good to kill off a badass query that reeked of Conditional Complexity. Now we’ve just got a simple loop, calling a simple query.

What's the moral of this story? Don't be afraid to get rid of big hairy badass queries that someone else wrote some time ago. You are smart enough to produce simpler code!