Simple Log Handler

Question

I want every piece of feedback that is possible as I am new to Java. I've coded some C# before but not much. I want to be sure that I am following the guidelines of how to write and name Java programs. Hit me as hard as you can.

I wrote this code for testing other functions I am writing, to track what is going on and when in the program. There are two classes one message class and one container class. Currently there are two constants stored in my final class.

It will later be used for logging in the program as well, so it is nothing temporary.

Constants.java

public final class Constants
{
 // log
 public static final int LogEntriesDisplay = 6;
 public static final int LogEntriesMax = 500;
}

Log.java

import java.util.List;
import java.util.ArrayList;
import java.util.Date;
import java.text.SimpleDateFormat;
/**
 * Log () default constructor; initializes a log
 * 
 * void AddEntry (String s) adds an entry at current time containing message s, removes oldest entry if entries.size() >= Constants.LogEntriesMax 
 * List<LogMessage> GetLogEntries () returns entries from offset to Constants.LogEntriesDisplay, never returns out of bounds
 * 
 * void IncOffset () increases offset by 1, never goes out of bounds
 * void IncOffset () decreases offset by 1, never goes out of bounds
 * void GoToTop () offset at start, never goes out of bounds
 * void GoToBottom () offset at end, never goes out of bounds
 * 
 * @author Emz
 * @version 1.0
 * @date 2014年11月15日
 */
public class Log
{
 private List<LogMessage> entries;
 private int offset;
 // default constructor; initializes a log
 public Log ()
 {
 entries = new ArrayList<LogMessage>();
 offset = 0;
 }
 // adds an entry at current time containing message s, removes oldest entry if entries.size() >= Constants.LogEntriesMax
 public void AddEntry (String s)
 {
 String t = new SimpleDateFormat("HH:mm:ss").format(new Date());
 LogMessage lm = new LogMessage(t, s);
 if (entries.size() >= Constants.LogEntriesMax)
 entries.remove(0);
 entries.add(lm);
 }
 // returns entries from offset to Constants.LogEntriesDisplay, never returns out of bounds
 public List<LogMessage> GetLogEntries ()
 {
 int fromIndex = offset;
 int toIndex = (offset + Constants.LogEntriesDisplay > entries.size()) ? entries.size() : offset + Constants.LogEntriesDisplay;
 return entries.subList(fromIndex, toIndex);
 }
 // increases offset by 1, never goes out of bounds
 public void IncOffset ()
 {
 offset = (offset < entries.size() - Constants.LogEntriesDisplay) ? offset+1 : offset;
 }
 // decreases offset by 1, never goes out of bounds
 public void DecOffset ()
 {
 offset = (offset == 0) ? 0 : offset-1;
 }
 // offset at start, never goes out of bounds
 public void GoToTop ()
 {
 offset = 0;
 }
 // offset at end, never goes out of bounds
 public void GoToBottom ()
 {
 offset = entries.size() - Constants.LogEntriesDisplay;
 }
}

LogMessage.java

public class LogMessage
{
 private String t;
 private String m;
 public LogMessage (String t, String m)
 {
 this.t = t;
 this.m = m;
 }
 public String GetTime ()
 {
 return t;
 }
 public String GetMessage ()
 {
 return m;
 }
}

Answer 1

Keep in mind that entries.remove(0) is an expensive operation on an ArrayList: the rest of the array (indexes 1...N) must be moved. If there are more removes than lookups (= if the log is normally at full capacity, so that every append operation triggers a remove(0)), then a LinkedList would be more efficient.

Having the state of an offset in this class violates the single responsibility principle. The class has 2 responsibilities now:

1. Add log entries
2. Manage state of an offset for viewing

You should split the class to separate these responsibilities. Let Log focus on logging only, and I suggest to rename it to Logger, to make it more specific and clear. Move the offset and its methods to another class, for example LogViewer. LogViewer can have a Logger, and an offset, and present a view from the Logger's entries based on the offset.

Avoid using a Constants class. It breaks encapsulation. Its current fields are only used by the logger class. It would seem best to make them private. If another class needs to know these values, they can get them from the logger class.

About naming, do follow the suggestions of @Sleiman Jneidi. Readability is very important.

• Emz: For a LinkedList, what is the exact difference of boolean add(Object o) and void addLast(Object o) ?
• janos: According to the javadoc they are equivalent.
• Emz: You recommend logviewer.getLogger().addEntry("New Entry") and logviewer.getLogger().getLogEntries((int)AnOffset) or should I implement addEntry (String) and getLogEntries() for LogViewer?
• janos: The LogViewer should be responsible for viewing logs, nothing else. It should not have an addEntry method. It should also not have a getLogger method. Remember the separation of responsibilities: one class should have one clear responsibility
• Emz: I also assume that all the .java files will be part of a logging package?

Answer 2

• When you move from a language to another, make sure you don't mix naming conventions. Unlike C#, Java uses Camel casing for method names and not Pascal casing. for example:

  public String getMessage(){
  ...
  }
  

  And not

  public String GetMessage ()
  {
   ..
  }
  

• Constants should be uppercased ( I mean its conventional)

  public static final int LOG_ENTRIES_DISPLAY = 6;
  

• t and m are not good names for fields representing time and message. You can simply call them time and message.
• String is not the right data structure for representing time, you can use Date instead where you can override the toString method to get a string out of your LogMessage.
• There is no need to initialize offset to zero in the constructor where it's already initialized to zero (because it is a field).

Answer 3

I agree with Sleiman's review, so I'll skip those points.

I'm not a fan of your Constants class.

• being a final class prevents any other implementations. If you decide later to change the window size or limits, you can't.
• the members being public static final int prevents any configuration at run time. Maybe you only care about 500 messages now, but what happens when that important message is 501 away?

I would recommend an interface with read-only properties (getters, no setters) and have your final class implement that interface and return its constant values.

I'm also not a fan of your Log class.

• it violates SRP. Is it a logger or a ring buffer? I would recommend using an existing ring buffer implementation for your storage in place of a List, or rolling your own if this is academic.
• there are already logging mechanisms built into the base Java libraries as well as the excellent log4j third party library. When learning new languages it is important to understand what the language provides.