Bad Singleton

I inherited a program the other day.  I was given a maintenance ticket.  I downloaded the program to my desktop and tried to start it.  I immediately got a bunch of NullPointerExceptions.  Endless NullPointerExceptions.  The NPEs were were emanating from any calls to a singleton.  I looked at the singleton's getInstance() method and found some interesting code:

public static XmlParser getInstance()
{
    if ( _instance == null )
    {
        try
        {
            _instance = new XmlParser();
        }
        catch ( Exception e )
        {
            log.error( e );
        }
    }
    return _instance;
}

First of all, exceptions were created for a reason.  We shouldn't just ignore them.  Ignoring exceptions makes it harder to debug problems.  To combat that, the developer added the log message.  That person didn't fully understand the Log4j api.  Instead of logging the exception, it logged the type of exception being thrown.  That isn't exactly helpful when you are trying to debug a problem.  Second, since this is a singleton getInstance, the null return caused the NPE to be thrown all over the place.  It would have been better to initialize the singleton at program startup so that we can fail it upfront, instead of allowing all the threads to start up.

Although this is a multi threaded program, I agree with the initial developer that it didn't need to be synchronized.  The class parses XML files.  It does not cache the parsed object.  Therefore, it doesn't matter too much if there are two instances in memory at the same time.

This was an interesting "gift".  Christmas did not come early.