First, I would separate loading the certificate from storing it.
- They are independent concerns.
- It allows multiple ways to load a certificate.
- It improves testability.
As Kinjal suggests, logging the error is a good alternative when failure is recoverable. However, I would throw a custom exception (or use CertificateException
) and let the caller decide to log and continue or terminate. I really don't like returning success codes when I can avoid it because it muddies the code with checks.
Finally, instead of setting a certificate or leaving the field blank consider modeling a certificate checking strategy. This would require a simple one-method interface CertificateCheck
with two implementations: NullCertificateCheck
and BasicCertificateCheck
. Again this improves testability by providing a seam for mocking, separating concerns, etc. It also removes the need for if (this.certificate != null)
checks and allows for more strategies and certificate types.
public void setupCertificate() {
try {
certificateCheck = loadX590Certificate(...);
}
catch (InvalidCertificateException e) {
LOG.warn("Skipping certificate check", e);
certificateCheck = new NullCertificateCheck();
}
}
private CertificateCheck loadX509Certificate(File file) throws CertificateException {
InputStream is = null;
try {
CertificateFactory cf = CertificateFactory.getInstance("X.509");
is = new FileInputStream(file);
return new BasicCertificateCheck(cf.generateCertificate(is));
}
catch (FileNotFoundException e) {
throw new CertificateException("Certificate file not found", e);
}
... more exceptions ...
finally {
IOUtils.closeQuietly(is);
}
}
Update: Since you're storing the certificate (or not) so you can check its validity, I assumed you were checking it at a later time and ideally multiple times. If this is the case, extract the logic into a CertificateCheck
hierarchy using the Strategy pattern. If the certificate is checked once in isolation of any other data, this isn't necessary and I see no reason to store the certificate in an instance field.
Note: I'm using Checkable
as a stand-in for whatever contains the information that must be checked against the certificate. Your implementation would need to pass whatever is appropriate.
public interface CertificateCheck {
boolean matches(Checkable thing);
}
// in the absence of a certificate, everything matches
public class NullCertificateCheck implements CertificateCheck {
public boolean matches(Checkable thing) {
return true;
}
}
public class BasicCertificateCheck implements CertificateCheck {
private final Certificate cert;
public BasicCertificateCheck(Certificate cert) {
this.cert = cert;
}
public boolean matches(Checkable thing) {
// move the current checks you have here
}
}
is.close()
in atry-catch
to separate important I/O errors while reading vs. those you can ignore when closing the stream. \$\endgroup\$