The Semi-Swallowed Exception

I am often brought in to help a team track down super-hard-to-find bugs. One situation that results in the super-hard-to-find bug is what I call the semi-swallowed-exception.

Most java developers know that it's a very bad idea (99% of the time) to completely swallow an exception like this:

try{
    ...
    ...
}catch(Exception e){}

That is, you very rarely want an empty catch block.

But almost as troublesome is this:

try{
    ...
    ...
}catch(Exception e){
    log.error(e);
}

The problem is, that the exception probably left the application in an invalid state. For example, some variable wasn't set correctly. Then, sometime later, you get a null pointer exception or worse, some strange hard to explain bug.

Based on the projects I have been involved with, I would say that 90% of time when you find yourself wanting to do this:

try{
    ...
    ...
}catch(Exception e){
    log.error(e);
}
...

Instead, you probably should do this:


try{
    ...
    ...
}catch(Exception e){
    log.error(e);
    throw new IllegalStateException("bla bla",e);
}

Also, if you are just catching an exception from a lower layer, there is not much value in wrapping it in a do nothing exception class. For example, I came across this the other day:


try{
    ...
    ...
}catch(Exception ex{
    log.error(ex);
    throw new MyCompanyNameException("bla bla");
}


Two things to note about this snippet:

1. The MyCompanyNameException class adds no value in this case. This seems to be a popular thing to do. But I can see no point to it.
    
2. This exception handler actually swallows information.The MyCompanyNameException includes a string message ("bla bla"). But it swallows the cause exception. So I would replace the above exception handler with this:

try{
    ...
    ...
}catch(Exception ex){
    log.error(ex);
    throw new RuntimeException("bla bla",ex);
}

Concurrent Maps for lightweight caching

Here is the Dave best practice when using a map for caching purposes in a java server environment:

Don't naively use Java maps as a global cache in a Java Server environment. 

Instead:

If it's a create-once, read-many type cache.

Use a Guava ImmutableMap and ImmutableMapBuilder:

public class DaveMapDemo1 {

   //note: Person is immutable
   private final ImmutableMap<String,Person> map;

   public DaveMapDemo1() {

       ImmutableMap.Builder<String,Person> builder =
               new ImmutableMap.Builder<String,Person>();
       builder.put("123",new Person("Dave","Ford"));
       builder.put("222",new Person("Joe","Blow"));
       map = builder.build();

   }

   public Person getPerson(String id){
       return map.get(id);
   }

}

If it's a get-or-create type cache.
If the cache is populated lazily, use Guava's LoadingCache and CacheBuilder classes (these replace the Guava ConcurrentMap implementations):

public class DaveMapDemo2 {

   //note: Person is immutable
   final LoadingCache<String, Person> map;

   public DaveMapDemo2() {

       map = CacheBuilder.newBuilder()
               .maximumSize(1000)
               .expireAfterWrite(10, TimeUnit.MINUTES)
               .build(
                       new CacheLoader<String, Person>() {
                           public Person load(String id) {
                               return lookupPersonFromDatabase(id);
                           }
                       });

   }

   public Person getPerson(String id) throws ExecutionException {
       return map.get(id);
   }

   private Person lookupPersonFromDatabase(String id) {
       // yada yada yada
   }
}

Summary
Notice that both examples:

1. Have no mutable state
2. Do not use the keyword synchronized