Sometimes you just have to ask why. Why would you ever produce this monstrosity?

void processMovies(List movieList, User user) {
  if (user.hasFavouriteMovies()) {
    Movie movie;
    while (movieList.hasNext()) {
      movie = movieList.next();
      switch (movie.getRating()) {
        case LOVED_IT:
          if (movie.hasExplosions()) {
            doLovedItWithExplosions(movie);
          } else {
            doLovedItWithoutExplosions(movie);
          }
         break;
        case HATED_IT:
          if (movie.hasExplosions()) {
            doHatedItWithExplosions(movie);
          } else {
            doHatedItWithoutExplosions(movie);
          }
          break;
        default;
          if (movie.hasExplosions()) {
            doIndifferentItWithExplosions(movie);
          } else {
            doIndifferentItWithoutExplosions(movie);
          }
      }
    }
  }
}

There is no good reason to have an if inside a switch. If you have to do a further conditional in the switch than either break it out into a function or get rid of the switch. It makes it unreadable, especially if it is nested inside two other layers of nesting as in the example above. That is too much to handle.

We can refactor this in multiple ways. I prefer the cases of my switch statement to be small so I will break out the logic into separate functions and thus reducing the cyclic complexity of that code. You could also translate the switch statement into an if/else if/else structure as well, but I do not find that to be as elegant.

void doLovedIt(Movie movie) {
  if (movie.hasExplosions()) {
     doLovedItWithExplosions(movie);
  } else {
     doLovedItWithoutExplosions(movie);
  }
}

/* doHatedIt, doInfferent */

void processMovies(List movieList, User user) {
  if (user.hasFavouriteMovies()) {
    Movie movie;
    while (movieList.hasNext()) {
      movie = movieList.next();
      switch (movie.getRating()) {
        case LOVED_IT:
          doLovedIt(movie);
          break;
        case HATED_IT:
          doHatedIt(movie);
          break;
         break;
        default;
          doIndifferentIt(movie);
          break;
      }
    }
  }
}

I find that to be much more readable, and it is easier to test. The new functions can be unit tested easily since they are isolated, and the processMovies function can be tested easier as well because it no longer have all the extra ifs to execute.

What a great way to begin the week. I have come across code from either a developer who is paid by the line, or someone who learned about the do … while statement.

  boolean hasNext = true;
  do {
      hasNext = object.hasNext();
      if  ( hasNext ) {
          // code
      }
  } while ( hasNext );

This is a lot of code that can be simplified to:

  while (object.hasNext()) {
      // code
  }

Java does not allow you to return multiple objects in a single function. To get around this, some people decide to collect all the data into an array and return that. I have to admit that I hate this pattern.

public static final int INDEX_NAME = 0;
public static final int INDEX_JOB = 1;
public Object[] getDetails() {
Object[] o = new Object[2];
o[INDEX_NAME] = name;
o[INDEX_JOB] = job;
return o;
}

The above pattern has one major problem. It ties you down to specific indexes for your data since you cannot guarantee that the invoker of this function will use constants provided by your class. This may not be an issue if it is private and never used anywhere outside of the class, but I like seeing my internal interfaces adhere to the same standards as the external ones.

However, this is not why I hate it. I find that it is ugly. It is readable with the proper use of constants, but it will require a casting operation, and data[INDEX_NAME] is just not as nice as data.getName();

Getting around these issues is as simple as defining a data class. I know it looks like a no brainer, but most great ideas do.
public class Info {
private String name;
private String job;
/** getters and setters */
}

public Info getDetails() {
Info I = new Info;
i.name = name;
i.job = job;
return i;
}

Not only is this more readable, but you are more free to modify the info class in the future if desired.

Generics are really easy to use badly. There are very few places in which they are a good idea, and most of them are implemented for us in the standard library. I ran across one of these usages recently that I cannot resist commenting on. (I’ll only be covering the generics in this post, and not nitpicking over the silliness of this methods existence. e.g. why is there a wrapper for ds to begin with?)

public static T getTypedValue(IDataStore ds, String key, Class clazz) {
if (clazz.equals(String.class)) {
return (T) ds.getString(key);
} else if (clazz.equals(Integer.class) {
return (T) ds.getInteger(key));
} else if (...) {
/* more casting */
}
return (T) ds.getObject(key);
}

// usage:
String value = getTypedValue(ds, "name", String.class);

There is no added type safety in the above code. The generics used here moves the casting from elsewhere in the code base to this method. This can definitely be improved upon.

Lets take a look on how this method would have been created without generics.

public static Object getTypedValue(IDataStore ds, String key, Class clazz) {
if (clazz.equals(String.class)) {
return ds.getString(key);
} else if (clazz.equals(Integer.class) {
return ds.getInteger(key));
} else if (...) {
/* more casting */
}
return ds.getObject(key);
}

// usage:
String value = (String) getTypedValue(ds, "name", String.class);

The method looks cleaner. There isn’t any generic syntax to add clutter, and no casting needed since it returns an object. The casting is done outside the method. I think it is nicer, but not nice enough.

The reason why the generics were introduced to this method is because they were trying to remove casting. This can be done by breaking this method down to smaller ones. Each with its own different return type.

public static String getStringValue(IDataStore ds, String key) {
return ds.getString(key);
}

// usage:
String value = getStringValue(ds, key);

I do beleive that this is the best way to go. The new methods are trivial and easy to read. There is no generic clutter. Plus there is no casting at all. It is a win on all fronts.

Static initializers in Java are very handy. They allow you to run initialization code when the JVM is loading the class.

Consider the following code sample:

public class PropertyManager
{
public static PropertyManager instance = new PropertyManager();

public String getProperty(String name) {
// code
}
}

I have seen similar code to this used in several production systems, and it works most of the time. However, if the code in the static initializer fails because of an uncaught Exception then the class will not be unloaded by the JVM, and an ExceptionInInitializerError is thrown. This is not be a big problem if you are writing a stand alone java application since the application will stop and the stack trace will be readily available. However, this is not the case if you are writing code for a J2EE server.

Most J2EE servers will report this error in the log and continue to load the rest of your application. The error will likely end up being missed because of the verboseness of the logs on startup. This means that when you application tries to use this class it will not be loaded in the JVM resulting in a NoClassDefFoundError. A very strange error indeed!

Be careful of your error handling in static initializers. If you are not careful, it may lead you on a wild goose chase.