0
\$\begingroup\$

I am trying to create logger wrapper which will be used by calling application installed on IBM WAS. Since I am using java util logger, I will have to add handlers for each logger programmatically.

Please share your views. I am more concerned about the way I am using file handlers and closing them.

Here is TransactionLogger class

import java.io.IOException;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.logging.FileHandler;
import java.util.logging.Handler;
import java.util.logging.Logger;

public class TransactionLogger
{
  private String appCode;
  private String externalKey;
  private String externalBatch;
  private String transactionId;
  private Date ruleExecutionStartTime;
  private Date ruleExecutionEndTime;
  private long ruleResponseTime;
  private Date DSStartTime;
  private Date DSEndTime;
  private long DSResponseTime;
  private Integer rulesFired;
  private Integer errors;
  private String userData;
  private String serverName;
  private Handler fh;



public String getAppCode()
  {
    return this.appCode;
  }

  public void setAppCode(String appCode)
  {
    this.appCode = appCode;
  }

  public String getExternalKey()
  {
    return this.externalKey;
  }

  public void setExternalKey(String externalKey)
  {
    this.externalKey = externalKey;
  }

  public String getExternalBatch()
  {
    return this.externalBatch;
  }

  public void setExternalBatch(String externalBatch)
  {
    this.externalBatch = externalBatch;
  }

  public String getTransactionId()
  {
    return this.transactionId;
  }

  public void setTransactionId(String transactionId)
  {
    this.transactionId = transactionId;
  }

  public Date getRuleExecutionStartTime()
  {
    return this.ruleExecutionStartTime;
  }

  public void setRuleExecutionStartTime(Date ruleExecutionStartTime)
  {
    this.ruleExecutionStartTime = ruleExecutionStartTime;
  }

  public Date getRuleExecutionEndTime()
  {
    return this.ruleExecutionEndTime;
  }

  public void setRuleExecutionEndTime(Date ruleExecutionEndTime)
  {
    this.ruleExecutionEndTime = ruleExecutionEndTime;
  }

  public long getRuleResponseTime()
  {
    return this.ruleResponseTime;
  }

  public void setRuleResponseTime(long ruleResponseTime)
  {
    this.ruleResponseTime = ruleResponseTime;
  }

  public Date getDSStartTime()
  {
    return this.DSStartTime;
  }

  public void setDSStartTime(Date startTime)
  {
    this.DSStartTime = startTime;
  }

  public Date getDSEndTime()
  {
    return this.DSEndTime;
  }

  public void setDSEndTime(Date endTime)
  {
    this.DSEndTime = endTime;
  }

  public long getDSResponseTime()
  {
    return this.DSResponseTime;
  }

  public void setDSResponseTime(long responseTime)
  {
    this.DSResponseTime = responseTime;
  }

  public Integer getRulesFired()
  {
    return this.rulesFired;
  }

  public void setRulesFired(Integer rulesFired)
  {
    this.rulesFired = rulesFired;
  }

  public Integer getErrors()
  {
    return this.errors;
  }

  public void setErrors(Integer errors)
  {
    this.errors = errors;
  }

  public String getUserData()
  {
    return this.userData;
  }

  public void setUserData(String userData)
  {
    this.userData = userData;
  }

  public String getServerName()
  {
    return this.serverName;
  }

  public void setServerName(String serverName)
  {
    this.serverName = serverName;
  }

  public String toString()
  {
    String hostName = "";
    try
    {
      String hostNameDerived = InetAddress.getLocalHost().getHostName();
      if (hostNameDerived.indexOf(".") > 0) {
        hostName = hostNameDerived.split("\\.")[0];
      }
    }
    catch (UnknownHostException uhe)
    {
      hostName = "Unknown";
    }
    if ((this.ruleExecutionEndTime != null) && (this.ruleExecutionStartTime != null)) {
      this.ruleResponseTime = (this.ruleExecutionEndTime.getTime() - this.ruleExecutionStartTime.getTime());
    }

    setServerName(hostName);

    if (this.ruleExecutionEndTime == null) {
      this.ruleExecutionEndTime = new Date();
    }
    if (this.ruleExecutionStartTime == null) {
      this.ruleExecutionStartTime = new Date();
    }
    if ((this.DSStartTime != null) && (this.DSEndTime != null)) {
      this.DSResponseTime = (this.DSStartTime.getTime() - this.DSEndTime.getTime());
    }
    if (this.DSStartTime == null) {
      this.DSStartTime = new Date();
    }
    if (this.DSEndTime == null) {
      this.DSEndTime = new Date();
    }
    SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss:SSS");
    return "," + this.appCode + "," + this.serverName + "," + this.externalKey + "," + this.externalBatch + "," + this.transactionId + "," + sdf.format(this.ruleExecutionStartTime) + 
      "," + sdf.format(this.ruleExecutionEndTime) + "," + this.ruleResponseTime + "," + sdf.format(this.DSStartTime) + "," + sdf.format(this.DSEndTime) + "," + this.DSResponseTime + "," + this.rulesFired + "," + this.errors + "," + this.userData;
  }

  public void logTransaction(Logger logger)
  {
    try
    {
      logger.info(this.toString());
      fh.close();

    }
    catch (Exception ex)
    {
      Logger errorLogger = Logger.getLogger("comcommon.logging.error");
      ex.printStackTrace();
      String message = "response Time log error:" + ex.getStackTrace();
      errorLogger.severe(message);
    }
  }

  // added this method to add file hander for each app.
  public void addFileHandler(Logger logger, String logFilePath)
  {
      try {
        //Handler fh = new FileHandler(logFilePath);
        fh = new FileHandler(logFilePath, 140000, 5, true);
        logger.addHandler(fh);



    } catch (SecurityException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }


  }


}

Here is how I am calling this logger from my application.

transactionLog = new common.logging.TransactionLogger(); 
log = Logger.getLogger("com.app.cin.log");
transactionLog.appCode = "CompleteInvoiceWithNewLogger" ; // app mnemonic 
transactionLog.addFileHandler(log,"C:/tmp/cin-response-time.log");
// Some other calculations for calculating response time and add that to log
transactionLog.ruleExecutionStartTime = new Date();
transactionLog.ruleExecutionEndTime = new Date(); 
transactionLog.logTransaction(log);
\$\endgroup\$
  • \$\begingroup\$ I think you have a copy-paste error. Please copy your exact code. \$\endgroup\$ – Simon Forsberg Feb 1 '18 at 16:20
  • \$\begingroup\$ Is there any reason you are not using slf4j, log4j, or any of those? \$\endgroup\$ – Simon Forsberg Feb 1 '18 at 16:20
  • \$\begingroup\$ Due to product restriction ( IBM ODM latest version) we cant use log4j. \$\endgroup\$ – JSR Feb 1 '18 at 18:44
  • \$\begingroup\$ Added complete code. \$\endgroup\$ – JSR Feb 1 '18 at 18:52
1
\$\begingroup\$

Here are my comments:

  1. TransactionLogger serves as both the log message data structure (with many properties), and as the output handler writing the log message. So you will need to modify the class whenever you change some log propeties and also whenever you change the process of writing to output. It is better to separate the two responsibilities to two classes, and preferebly with interfaces that define the contract between these two responsibilities so that they can be extended to accomodate future requirements (for example, writing log to DB or other destinations, different lof formatting etc)

  2. The application code that uses TransactionLogger does not call the setter methods. not only this is bad design, but I don't see how this passes compilation.

  3. While we are at the issue: the log message has many properties with setters. a (working) application code that has to call each setter will look long and ugly. I suggest to either use the Builder pattern, or the fluent method chaining technique

  4. The toString() does a lot of work. once again, this creates the risk of being hard to maintain and extend in the future. You can start by having a separate getHostName() and continue breaking up the code to smaller pieces that do "atomic" operations like determining value of one attribute

  5. Dates: First of all, you initialize the SimpleDateFormat with every call to toString(). This class makes heavy use of expensive regex processing. It should be moved to be a static final variable.

  6. Second, Java 8 contains a new date time api, similar to the one in Joda library. it handles dates better, including formatting them into Strings.

  7. Last but certainly not least: you close the file handler only if no excepption is thrown, creating the risk of resource leak. You should close the handler in a finally clause or better still, use Java 7 try-with-resources feature

\$\endgroup\$
  • \$\begingroup\$ if you think the answer is useful, please accept and/or upvote it \$\endgroup\$ – Sharon Ben Asher Feb 7 '18 at 8:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.