Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

We have a few servlets like the one below, taking JSON request and produce JSON response. And the requirement is to log all API call traffic.

import org.slf4j.*;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
public class CommentServlet {
  private static final Logger LOG = LoggerFactory.getLogger(CommentServlet.class);
  public void postComment(HttpServletRequest request, HttpServletResponse response) throws IOException {
    String commentRequested = request.getParameter("comment");
    LOG.info(commentRequested);
    Object postResultModel = processComment(commentRequested);
    JsonUtil.writeResponse(response, postResultModel);
  }
  /**
  * doesn't reallty matter what processComment does
  */
  public Object processComment(String comment) {
    return new Object();
  }
}
import org.slf4j.*;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.IOException;
public class JsonUtil {
    private static final Logger LOG = LoggerFactory.getLogger(JsonUtil.class);
    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
    public static void writeResponse(HttpServletResponse response, Object modelObject) throws IOException {
        String modelJson = OBJECT_MAPPER.writeValueAsString(modelObject);
        LOG.info(modelJson);
        response.setContentType("application/json");
        response.getWriter().write(modelJson);
    }
}

if you can mainly review how the logging is done, I'd like feedback on the general approach as much as whether there's any way to improve it.

share|improve this question
2  
Questions containing hand-wavy code and asking us to speculate or extrapolate do tend to get downvoted. Rev 8 does look better. – 200_success Nov 9 '16 at 21:52
private static final Logger LOG = LoggerFactory.getLogger(CommentServlet.class);

This is considered as bad practice ... of course there is an argument from the other side as well.

The right thing to use should be

private final Logger LOG = LoggerFactory.getLogger(getClass()); 

This basically helps you leverage the capabilities of polymorphism and use the same logger in inherited classes with its real name.

share|improve this answer
    
Welcome to Code Review, your first answer looks good, but perhaps you could clarify the downside of the proposed fix, if there's one as mentioned in the first sentence. Perhaps also a link if there's already some more documentation on this. Enjoy your stay! – ferada Nov 10 '16 at 13:23

IMHO, the problem is that all the responses are using the same JsonUtil logger, and each request is using the logger of each Servlet. This is making it difficult to configure meaningful appender for logs. (imagine if you want to configure "comment" feature as mentioned above to have certain log level and write to a comment.log file).

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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