Turning 30 lines of code into 1

I attended a coworker's code review for some new functionality that was added to FooDaoHibernateImpl.java. It didn't make me happy. We've all written bad code at some point in our careers. The code you are about to see is from somebody who has been programming for over a dozen years. I would be more forgiving if it was somebody fresh out of high school who just learned Java. But that description doesn't fit this case.

With no apologies to the developer that wrote it, I'm embarrassed to have to show his code here. I don't want you to think this is the way we do things around here, but I really need to vent and maybe you can all learn something from this. Hopefully the developer that wrote it learned something.

I'll intersperse it with pseudocode to protect the guilty.

private static final findGenericFoo = "sql select statement where x = true";
private static final findNonGenericFoo = "sql select statement where x = false";

public List<FooRef> getGenericFooTypes() {
  fooAllReferrals = EMPTY_LIST;
  List<Object[]> genericFoos = //hibernate to findGenericFoo
  if genericFoos not empty {
      List<Object[]> nonGenericFoos = //hibernate to findNonGenericFoo
      if nonGenericFoos not empty {
          fooAllReferrals = mergeGenericAndNonGenericFoos(genericFoos, nonGenericFoos);
      }
  }
  return fooAllReferrals;
}

List<FooRef> mergeGenericAndNonGenericFoos(List<Object[]> generic,List<Object[]> nonGeneric) {
  List<FooRef> fooTypeRef = new ArrayList<FooRef>();
  for loop through generic {
      genericFooId = Long.valueOf(String.valueOf(fooGeneric[0]));
      for loop through nonGeneric {
          fooId = Long.valueOf(String.valueOf(fooNonGeneric[0]));
          if fooId.longValue == genericFooId.longValue {
              if fooGeneric[1].toString().equals(fooNonGeneric[2].toString()) {
                  FooRef oneFooType = new FooRef();
                  oneFooRef.setFooId(Long.valueOf(String.valueOf(fooNonGeneric[0]));
                  // etc for fooNonGeneric[1]
                  // etc for fooNonGeneric[2]
                  fooTypeRef.add(oneFooType);
              }
          }
      }
  }
  return fooTypeRef;
}

Now where do I even begin to talk about all the things that are wrong with this? I'll skip mentioning the notion that when you have a List of something that the name should be plural because there's only so much space in a blog post and I want to stick to the major problems.

1. Why perform a merge of two SQL queries in code? The database has really efficient ways of joining. Use the database to do the work for you. Don't know how to write the SQL for it? Find somebody who does and learn how.
2. Using a list of object arrays (List<Object[]>) is frowned upon. We're using Hibernate! It can map query results directly to objects for us! Use the power tools.
3. Long.valueOf(String.valueOf(something[1])). I sure hope the order of the values coming back in the SQL statement don't ever change. This relates to the previous one. The only reason that this is even going to work is because you know what those values are.
4. FooRef oneFooType = new FooRef(); oneFooType.setX(); oneFooType.setY(); etc, etc... is funny to read. Why use the default constructor and then to three (or more) set()'s on it? Make a new constructor. Or use the builder pattern. We can do better than set, set, set.

I wanted to cry when I saw that code. Sure it works (actually if there were no genericFoos then you wouldn't get the nonGenericFoos). But it's ugly code that's going to be difficult to maintain and read. After much instruction on what to fix the developer worked on it for a couple days. The result was much better:

private static final FIND_GENERIC_FOOS = "select new FooRef(...) where...";

public List<FooRef> getGenericFooTypes() {
   List<FooRef> fooTypeReferral = ListUtils.EMPTY_LIST;
   fooTypeReferral = getSession().createQuery(FIND_GENERIC_FOOS).list();
   return fooTypeReferral;
}

Much improved, but it could be still better. Why create a variable just to assign something to it and return it? I explained to the developer that assigning EMPTY_LIST to fooTypeReferral doesn't do any good since it just gets reassigned by the Hibernate list() method. I believe that's what they call a Dead Store. So he then turned it into a single line and I was much happier. At least for a while. Makes me wonder what the next code review will bring.