Logging conditionally repetitively in code

Question

I am using Log4j2 for logging. Multiple operations return success code. If status 0 is received, it means operation is successful, else failed. If operation is successfully, I log "*** is successfully." and if it fails I logs "*** failed with status : " + status. This is kind of repetitive in code.

public class ReadCallbackManager {

  //Log Object
  private static final Logger LOGGER = LogManager.getLogger(ReadCallbackManager.class);

  private static ReadCallback readCallback;

  private static final int SUCCESS_STATUS = 0;

  public int setHook(final ICallback aCallBack) {

    readCallback = new ReadCallback(aCallBack);

    final int status = readCallback.setHook();

    if (status == SUCCESS_STATUS) {
      LOGGER.info("Read callback hook is set successfully!!!");
    } else {
      LOGGER.info("Read Callback hook set failed with status : " + status);
    }

    return status;

  }

}

So I though to create a wrapper around the Logger class to handle this if else condition as below:

public final class BacStacLogger {

  public static final int SUCCESS_STATUS = 0;

  //Log Object
  private Logger logger;

  public BacStacLogger(final Class<?> className) {
    if (logger == null) {
      logger = LogManager.getLogger(className);
    }
  }

  public void infoHookByStatus(final int status, final String successMessage, final String failureMessage) {
    if (status == SUCCESS_STATUS) {
      logger.info(successMessage);
    } else {
      logger.info(failureMessage + " : " + status);
    }
  }

}

And I have updated my previous code as :

public class ReadCallbackManager {

  //Log Object
  private static final ProjectLogger LOGGER = ProjectLogger(ReadCallbackManager.class);

  private static ReadCallback readCallback;

  public int setHook(final ICallback aCallBack) {

    readCallback = new ReadCallback(aCallBack);

    final int status = readCallback.setHook();

    LOGGER.infoHookByStatus(status, "Read hook set successfully.", "Read hook set failed.");

    return status;

  }

}

Would you please provide your comments on it? Or if required, any improvements?

Answer 1

There are a few remarks that come to my mind.

Simplifying the logic

In your sample code, the two different log strings for success and failure don't differ too much. So I guess the following strings might be acceptable to you as well:

"Setting read callback hook: successful"
"Setting read callback hook: FAILED with status nnn"

Then you can create a static method statusText(int status) e.g. in a Utils class that returns "successful" for status==0 and "FAILED with status nnn" in all other cases. If I understood correctly, such a statusText() method should be re-usable all over your application, assuming that 0 always means success.

And then you can do the logging like:

LOGGER.info("Setting read callback hook: " + Utils.statusText(status));

But there are a few other issues:

Log Levels

Log4J has different log levels, and you shouldn't ignore that aspect. Depending on the consequences, a failure should typically get the ERROR level, at least the WARN level. So, in case of a non-zero status, LOGGER.error() seems to be a better choice.

INFO is meant for messages useful to the system administrator running the site. Messages targetting the developer should get a lower level, typically DEBUG or TRACE. I guess, the success of setting some callback isn't important to the system admin. The admin typically only wants to see things needing his/her special attention. So, I'd change the success case to LOGGER.debug().

Of course, then my statusText() solution no longer fits, as that doesn't support different log levels.

Why not Exceptions?

Your code sticks to the 1970s status code pattern to communicate failure (return an integer where some values mean success and others mean failure). By the 1990s, the software industry had learned that error handling can be done in a better way using exceptions.

So, instead of returning int values and forcing each and every layer of your software to check for the success results, throw an exception if something like setting a callback failed. By embracing exceptions as the signal of failure, your code typically becomes

• cleaner,
• more readable,
• more compact,
• more focussed on the main task instead of the failures
• and more robust.

The simple guidelines for exceptions are:

• If a method can't fulfill its job (whatever you defined that job to be), it throws an exception.
• If a method returns normally, the caller can rely on the fact it has successfully done its job.
• You catch an exception only if you know a way how to continue successfully even after an internal failure, e.g. by retrying.