0
\$\begingroup\$

I have written a Custom Log4J Appender which is responsible for sending the log message to a third party service asynchronously. It is expected to be called every millisecond.

I ran FindBugs on this code, it didn't report anything unusual. But before releasing this code to production, how can I know if this code has any issues with it?

package com;

import java.util.ArrayList;
import java.util.Iterator;

import org.apache.log4j.AppenderSkeleton;
import org.apache.log4j.spi.LoggingEvent;

public class MyAppender extends AppenderSkeleton {

    public MyAppender() {

    }

    public void append(LoggingEvent event) {
        ArrayList<LoggingEvent> buffer = new ArrayList<LoggingEvent>();
        buffer.add(event);
        flushBuffer(buffer);
    }

    public void flushBuffer(ArrayList<LoggingEvent> buffer) {
        for (Iterator<LoggingEvent> i = buffer.iterator(); i.hasNext();) {
            try {
                LoggingEvent logEvent = (LoggingEvent) i.next();
                String messageRecievied = (String) logEvent.getMessage();
                //System.out.println(messageRecievied);

            } catch (Exception e) {

            }

        }

    }

    @Override
    public void close() {

    }

    @Override
    public boolean requiresLayout() {

        return false;
    }

}
\$\endgroup\$
  • \$\begingroup\$ If you want a very simple check if whether your code has any issues then test it against this very simple checklist codinghorror.com/blog/2006/05/code-smells.html Code smells As you gain experience with that you will find locationg code smells easier \$\endgroup\$ – Marco Forberg Jun 22 '13 at 5:06
2
\$\begingroup\$

First, I don't think the close() method is following the contract, from the javadoc:

It is a programming error to append to a closed appender.

Therefore, your close() method should be implemented like this:

@Override
public void close() {
    this.closed = true;
}

Note that closed is a protected field in AppenderSkeleton.

Then, change append to something like (add the @Override as well):

@Override
public void append(LoggingEvent event) {
    if (!this.closed) {
        ArrayList<LoggingEvent> buffer = new ArrayList<LoggingEvent>();
        buffer.add(event);
        flushBuffer(buffer);
    } else {
        // It is a programming error to append to a closed appender.
        throw new RuntimeException("Attempting to append to a closed Appender.");
    }
}

Next, does flushBuffer need to be public? It should probably be private.

Finally—and this is more of a nitpick—you don't need to declare the default constructor since it's empty and the compiler will create one for you. It'll make your code a little cleaner as well.

\$\endgroup\$
1
\$\begingroup\$

Add source code comments. Why does the class exist? What does it do? What do each of the methods do? What are their pre- and post-conditions?

This code:

ArrayList<LoggingEvent> buffer = new ArrayList<LoggingEvent>();

Should be:

List<LoggingEvent> buffer = new ArrayList<LoggingEvent>();

Similarly:

public void flushBuffer(ArrayList<LoggingEvent> buffer) {

As:

public void flushBuffer(List<LoggingEvent> buffer) {

Next up:

        } catch (Exception e) {

        }

Allow that Exception to bubble up:

public void flushBuffer(List<LoggingEvent> buffer) throws Exception {

But, be as specific as possible about the exception that can be thrown (i.e., if isn't Exception itself that is thrown, but a subclass, then opt to throw that subclass instead).

\$\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.