Avoid actually/logically unreachable code in catch clause

Question

I have an interface :

public interface FileRepository {
    String insert(File file) throws IOException;
    // other methods ...
}

My implementation of insert(File file) uses local (to avoid concurrency problem) java.security.MessageDigester which throws checked exception java.security.NoSuchAlgorithmException from its factory method.

public FileRepositoryImpl(String digestAlgo) throws NoSuchAlgorithmException {
    this.digestAlgo = digestAlgo;
    MessageDigest.getInstance(digestAlgo);
    }
@Override
public String insert(File file) throws IOException {
// initialize message digest
    MessageDigest messageDigest = null;
    try {
        messageDigest = MessageDigest.getInstance(digestAlgo);
    } catch (NoSuchAlgorithmException e) {
        LOGGER.fatal(MD_INIT_ERROR, e);
        return null;
}
    // other code ....
}
// other methods (may contain local MessageDigest)

My practice: As NoSuchAlgorithmException always be a fatal error (which makes the module totally unavailable), I try to initialize a MessageDigest in my constructor to test the parameter digestAlgo, so the exception can be thrown by the constructor, other (earlier) than from insert(File). Another reason is the interface don't allow throwing NoSuchAlgorithmException by definition.

My question: in my implementation, the code

   } catch (NoSuchAlgorithmException e) {
        LOGGER.fatal(MD_INIT_ERROR, e);
        return null;
   }

will never be reached, so I think there should be better solutions, which allows to avoid the (logically and practically) unreachable code.

Any solution/suggestion will be welcome, thanks.

Edit:

It is not really an issue when running the code. But in test, as the code is unreachable, together with some "try-catch with resources", quality analyzation tool (sonar,pmd) will consider the code "Insufficient branch coverage by unit tests", and this is a major issue in the analyze report, that's why I want to avoid this piece of code.

Another question, is it a good practice to test MessageDigest.getInstance(digestAlgo); in my constructor? Or it's better to let insert(File) take full responsibility of the NoSuchAlgorithmException?

Answer 1

There is too much going on with this class. If it is, as its name suggests, a file repository its focus should be on storing files, but there is clearly a lot of unrelated activity around getting a MessageDigest instance--should it be this class' responsibility to create a kind of tunneled (via static method) dependency on the MessageDigest? There are a number of options for dependency injection all of which enable you to off load the responsibility to configure your objects to a framework dedicated to that purpose (Spring, Guice, PicoContainer, etc.).

I think you recognize the problem here, so that's a great start. As a rule you should be striving to not throw exceptions and usually object constructors are among my least favorite places to do so. The awkwardness you face here can go away completely if you make use of a framework to help you configure your objects. Also, not once but twice you are returning null--it will make you check for null on the other end of the method call and do you really want to do that? If you give this some thought, I bet you will want to find another way (and you do have other options).

Also, you'll find that configuring your objects with a specialized object creation factory (which is what those dependency injection frameworks are) will help you couple your components together much more loosely and this will promote the possibility of testing the components in isolation from their real dependencies--use mocks for testing. If you can make a shift in thinking where you're writing unit tests for the code you wish you had before you do anything else, you should expect to see much better designs begin to happen almost unconsciously. The GOOS book is a great resource to start with. Best wishes!

EDIT: I misread--looks like you return null just once, but that's still one time too many. :-/

Answer 2

I think I would change the constructor to take a MessageDigest instead of an algorithm name, and let the caller deal with the exception. But there's another issue here. Having the digester as an instance member means that the class isn't thread-safe.