3
\$\begingroup\$

I have converted this code from Java to Scala, and I need reviews. This works fine, but I am sure that something is wrong with this coding pattern.

package models

import org.apache.log4j.ConsoleAppender
import org.apache.log4j.Level
import org.apache.log4j.Logger
import org.apache.log4j.PatternLayout
import org.apache.log4j.RollingFileAppender

import me.prettyprint.cassandra.serializers.StringSerializer

object LogFile extends CassandraConnection {

  val loger = Logger.getRootLogger()
  def exceptionLogs(qName: String, data: String) {
    //data is json String
    val logg = Logger.getLogger(qName + "-" + data)
    loger.setLevel(Level.ERROR)
    val layout = new PatternLayout("[%t] %-5p %c %x - %m %d{ISO8601} %n")
    loger.addAppender(new ConsoleAppender(layout))
    try {
      val fileAppender = new RollingFileAppender(layout, "QLog.txt")
      loger.addAppender(fileAppender)
      logg.error("")
      loger.removeAppender(fileAppender)
    } catch {
      case e: Exception =>
        println(e)
        logg.error("");
    }
  }

}
\$\endgroup\$
  • \$\begingroup\$ This feels like it's configuring Log4j more than it's doing logging. XML files or a .properties file are meant to be used for configuring Log4j. I would say that there is indeed something wrong with this coding pattern. \$\endgroup\$ – Simon Forsberg Jun 28 '14 at 20:45
5
\$\begingroup\$

I'm not exactly sure what you mean with coding pattern, but there are some things that strike me as odd:

  1. you only log empty messages ... in some of the cases encoding the message in the logger name. This certainly isn't the way log4j is intended to be used.

  2. When ever you call this method basically the same Appender is created added and removed again. This again is a strange way to use log4j. And I'm not even sure what will happen when two threads call this method at the same time.

  3. You are setting the ConsoleAppender and the Pattern over and over again. This at least is superfluous, and might actually be harmful.

  4. The naming is ... suboptimal ... You have two loggers one, is missing a g in its name, the other is abbreviated. None really tells the reader what these loggers are about.

  5. You never ever actually log with loger

So if this doesn't depend on some weird side effect this logs one empty String when it is called and two empty Strings when creating/adding/removing an Appender throws and exception. If this is really what this is supposed to do, it better made it clear in its names and comments.

\$\endgroup\$
1
\$\begingroup\$
  • object LogFile extends CassandraConnection looks very odd.

  • So are loger and logg.

  • layout and fileAppender should probably be static instead of being recreated at every method call.

  • I can't make sense of the code. I don't know log4j that well, but nonetheless, I think I should be able to vaguely understand what is happening. So I can't quite comment on the "coding pattern".

\$\endgroup\$

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.