6
\$\begingroup\$

My goal with the following code is to provide an extensible class that can be extended in order to read log files and transforms them into meaningful output.

I'd like to have comments about the code itself, the test code, the javadoc and the overall structure.

/**
 * Exception to indicate that there is no more input.
 *
 * @author Frank van Heeswijk
 */
public class NoMoreInputException extends Exception {
    private static final long serialVersionUID = -4640787627068619913L;

    public NoMoreInputException() {

    }

    public NoMoreInputException(final String message) {
        super(message);
    }

    public NoMoreInputException(final String message, final Throwable cause) {
        super(message, cause);
    }

    public NoMoreInputException(final Throwable cause) {
        super(cause);
    }
}

/**
 * Exception to indicate that a log entry is not readable.
 *
 * @author Frank van Heeswijk
 */
public class NotReadableException extends Exception  {
    private static final long serialVersionUID = -117259271357929934L;

    private final List<String> lines = new ArrayList<>();

    /**
     * Constructs a new NotReadableException instance.
     *
     * @param lines The lines that were not readable
     */
    public NotReadableException(final List<String> lines) {
        this.lines.addAll(lines);
    }

    /**
     * Returns the lines that were not readable.
     *
     * @return  The lines that were not readable.
     */
    public List<String> getLines() {
        return new ArrayList<>(lines);
    }
}

/**
 * Exception to indicate that a log entry is not parsable.
 *
 * @author Frank van Heeswijk
 */
class NotParsableException extends Exception {
    private static final long serialVersionUID = 3147294996191143729L;

    NotParsableException() {

    }

    NotParsableException(final String message) {
        super(message);
    }

    NotParsableException(final String message, final Throwable cause) {
        super(message, cause);
    }

    NotParsableException(final Throwable cause) {
        super(cause);
    }
}

/**
 * Used to read log entries from log files.
 *
 * @author Frank van Heeswijk
 */
public interface LogReader {
    /**
     * Returns the next log entry.
     *
     * If you get a NotReadableException, then you can recover the lines that could not be read by calling NotReadableException#lines.
     *
     * @return  The next log entry.
     * @throws NotReadableException If the log entry could not be read.
     * @throws NoMoreInputException If there is no more input.
     */
    LogEntry readEntry() throws NotReadableException, NoMoreInputException;
}

/**
 * Used to read lines from an input source.
 *
 * This functional interface differs from Supplier<String> in that it can throw a NoMoreInputException.
 *
 * @author Frank van Heeswijk
 */
@FunctionalInterface
public interface LineReader {
    /**
     * Reads a line.
     *
     * @return  The read line.
     * @throws NoMoreInputException If no more input could be obtained.
     */
    String readLine() throws NoMoreInputException;
}

/**
 * Used to read entries from log files.
 *
 * It has the option to read more lines from the line reader if deemed necessary.
 *
 * @author Frank van Heeswijk
 */
public interface EntryReader {
    /**
     * Returns whether this entry reader can parse the input.
     *
     * @param input The input check parsability for
     * @return  Whether this entry reader can parse the input.
     */
    boolean isParsable(final String input);

    /**
     * Parses the input String resulting in a LogEntry.
     *
     * If deemed necessary, extra lines may be obtained from the LineReader.
     *
     * @param input The input to parse
     * @param lineReader    The line reader from which extra lines can be obtained
     * @return  The LogEntry obtained after parsing the input.
     * @throws NotParsableException If this entry reader cannot parse the input to return a LogEntry.
     * @throws NoMoreInputException If the line reader has no more input.
     */
    LogEntry parse(final String input, final LineReader lineReader) throws NotParsableException, NoMoreInputException;
}

/**
 * Can be extended to read lines from log files.
 *
 * @author Frank van Heeswijk
 */
public abstract class AbstractLineLogReader implements LogReader {
    private final Set<EntryReader> entryReaders = entryReaders();
    private final List<String> linesInMemory = new ArrayList<>();

    @Override
    public LogEntry readEntry() throws NotReadableException, NoMoreInputException {
        String line = readLineFromLogAndSave();
        for (EntryReader entryReader : entryReaders) {
            if (!entryReader.isParsable(line)) {
                continue;
            }
            try {
                LogEntry result = entryReader.parse(line, this::readLineFromLogAndSave);
                linesInMemory.clear();
                return result;
            } catch (NotParsableException | NoMoreInputException ex) {
                //try next entry reader
            }
        }
        List<String> notReadableLines = new ArrayList<>(linesInMemory);
        linesInMemory.clear();
        throw new NotReadableException(notReadableLines);
    }

    /**
     * Returns the set of entry readers used to read the log files.
     *
     * @return  The set of entry readers used to read the log files.
     */
    protected abstract Set<EntryReader> entryReaders();

    /**
     * Returns the next line from the log file.
     *
     * @return  The next line from the log file.
     * @throws NoMoreInputException If no more lines are present.
     */
    protected abstract String readLineFromLog() throws NoMoreInputException;

    /**
     * Returns the next line from the log file and saves it.
     *
     * @return  The next line from the log file.
     * @throws NoMoreInputException If no more lines are present.
     * @throws java.lang.NullPointerException   If line is null.
     */
    private String readLineFromLogAndSave() throws NoMoreInputException {
        String line = readLineFromLog();
        Objects.requireNonNull(line, "line");
        linesInMemory.add(line);
        return line;
    }
}

public class AbstractLineLogReaderTest {
    @Test
    public void testReadEntryExpectedEntries() throws NoMoreInputException, NotReadableException {
        LogReader logReader = new ABCListLineLogReader("A", "B");

        assertEquals(ALogEntry.class, logReader.readEntry().getClass());
        assertEquals(BLogEntry.class, logReader.readEntry().getClass());
    }

    @Test(expected = NoMoreInputException.class)
    public void testReadEntryNoMoreEntries() throws NoMoreInputException, NotReadableException {
        LogReader logReader = new ABCListLineLogReader("A", "B");

        logReader.readEntry();
        logReader.readEntry();
        logReader.readEntry();
    }

    @Test
    public void testReadEntryNoReadersAvailable() throws NoMoreInputException {
        LogReader logReader = new EmptyListLineLogReader("A", "B");

        try {
            logReader.readEntry();
            fail();
        } catch (NotReadableException ex) {
            assertEquals(1, ex.getLines().size());
            assertEquals("A", ex.getLines().get(0));
        }
    }

    @Test
    public void testReadLogEntrySpanningMultipleLines() throws NoMoreInputException, NotReadableException {
        LogReader logReader = new ABCListLineLogReader("A", "C", "1","2", "3", "B");

        assertEquals(ALogEntry.class, logReader.readEntry().getClass());
        assertEquals(CLogEntry.class, logReader.readEntry().getClass());
        assertEquals(BLogEntry.class, logReader.readEntry().getClass());
    }

    @Test
    public void testReadLogEntrySpanningMultipleLinesLastEntryNotReadable() throws NoMoreInputException, NotReadableException {
        LogReader logReader = new ABCListLineLogReader("A", "B", "C", "1","2");

        assertEquals(ALogEntry.class, logReader.readEntry().getClass());
        assertEquals(BLogEntry.class, logReader.readEntry().getClass());

        try {
            logReader.readEntry();
            fail();
        } catch (NotReadableException ex) {
            assertEquals(Arrays.asList("C", "1", "2"), ex.getLines());
        }
    }

    @Test
    public void testReadLogEntrySpanningMultipleLinesIncorrectInput() throws NoMoreInputException, NotReadableException {
        LogReader logReader = new ABCListLineLogReader("A", "C", "1","2", "4", "B");

        assertEquals(ALogEntry.class, logReader.readEntry().getClass());

        try {
            logReader.readEntry();
            fail();
        } catch (NotReadableException ex) {
            assertEquals(Arrays.asList("C", "1", "2", "4"), ex.getLines());
        }

        assertEquals(BLogEntry.class, logReader.readEntry().getClass());
    }

    private abstract static class ListLineLogReader extends AbstractLineLogReader {
        private final Iterator<String> iterator;

        private ListLineLogReader(final String... inputList) {
            this(Arrays.asList(inputList));
        }

        private ListLineLogReader(final List<String> inputList) {
            Objects.requireNonNull(inputList, "inputList");
            this.iterator = inputList.iterator();
        }

        @Override
        protected String readLineFromLog() throws NoMoreInputException {
            if (!iterator.hasNext()) {
                throw new NoMoreInputException();
            }
            return iterator.next();
        }
    }

    private static class ABCListLineLogReader extends ListLineLogReader {
        private ABCListLineLogReader(final String... inputList) {
            super(inputList);
        }

        private ABCListLineLogReader(final List<String> inputList) {
            super(inputList);
        }

        @Override
        protected Set<EntryReader> entryReaders() {
            return new HashSet<>(Arrays.asList(
                new EntryReader() {
                    @Override
                    public boolean isParsable(String input) {
                        return input.equals("A");
                    }

                    @Override
                    public LogEntry parse(String input, LineReader lineReader) throws NotParsableException, NoMoreInputException {
                        if (!input.startsWith("A")) {
                            throw new NotParsableException();
                        }
                        return new ALogEntry();
                    }
                },
                new EntryReader() {
                    @Override
                    public boolean isParsable(String input) {
                        return input.equals("B");
                    }

                    @Override
                    public LogEntry parse(String input, LineReader lineReader) throws NotParsableException, NoMoreInputException {
                        if (!input.equals("B")) {
                            throw new NotParsableException();
                        }
                        return new BLogEntry();
                    }
                },
                new EntryReader() {
                    @Override
                    public boolean isParsable(String input) {
                        return input.equals("C");
                    }

                    @Override
                    public LogEntry parse(String input, LineReader lineReader) throws NotParsableException, NoMoreInputException {
                        if (!input.equals("C")) {
                            throw new NotParsableException();
                        }
                        if (!Objects.equals("1", lineReader.readLine())) {
                            throw new NotParsableException();
                        }
                        if (!Objects.equals("2", lineReader.readLine())) {
                            throw new NotParsableException();
                        }
                        if (!Objects.equals("3", lineReader.readLine())) {
                            throw new NotParsableException();
                        }
                        return new CLogEntry();
                    }
                }
            ));
        }
    }

    private static class EmptyListLineLogReader extends ListLineLogReader {
        private EmptyListLineLogReader(final String... inputList) {
            super(inputList);
        }

        private EmptyListLineLogReader(final List<String> inputList) {
            super(inputList);
        }

        @Override
        protected Set<EntryReader> entryReaders() {
            return new HashSet<>();
        }
    }

    private static class ALogEntry implements LogEntry { }

    private static class BLogEntry implements LogEntry { }

    private static class CLogEntry implements LogEntry { }
}

A snapshot of my Github repository containing this code can be found here.

\$\endgroup\$
8
\$\begingroup\$

NoMoreInputException

"No more input" shouldn't be an exception, as it's part of the normal flow and not an unexpected condition. Use the same paradigm as Iterator and add hasNext() to LineReader and LogReader.

LineReader

I don't see any compelling reason not to prefer the Iterator<String> interface.

EntryReader

Should this be named EntryParser? It doesn't read, it parses. Or really, it does both, which is messy.

AbstractLogLineReader

linesInMemory

I don't like this, just because it adds state to a class which could otherwise be stateless. There should be a way to refactor it out.

readEntry()
  1. Assumes that each entry is parseable by exactly one EntryReader. You may want to consider finishing the loop and throwing an error if multiple parsers are valid. The risk is using the wrong parser when multiples will work.
  2. Should be final, since you hopefully don't want subclasses to override this method.

General

As a user of this API subset, I guess I feel like there are a lot of moving pieces for me to worry about. I'd like to see one final class, LogReader, which either takes a BufferedReader containing a log file, a 'File' which it uses to build a Reader, or it figures out the file location for itself. It would have a default set of EntryParsers attached, and only if it makes sense give me a way to register more (directly on the object via constructor or addParser(), or some reflection/discovery process). It would have a method read() which returned an Iterator<LogEntry>. Everything else would be behind the scenes, because I don't care about any of the rest of it.

The implementation would then need to build that Iterator<LogEntry>. You might want something that can look at the log and pull lines until it has read a whole log entry, if possible, then pass that to the parser. You may be able to leverage BufferedReader#mark() and #reset(), depending on how performance-critical this is. Otherwise you're stuck with the "first line plus Reader" parameters to the parser, which is not my favorite but not always avoidable with multiline log files.

All of those exceptions should probably go away in favor of IOException, since you shouldn't be using them for business logic. It may be appropriate to throw a specific exception, but I'm not sold based on what you've shown. You may need a LogEntryNotParseable exception, I suppose. In that case you need one custom interface that's basically Iterator<LogEntry> but next() would throw your exception. That lets you keep walking through the rest of the log entries if one can't be parsed.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for your comments, I really appreciate them. I am right now in the process of getting my reader to work (almost done!), and then I'll start refactoring it based on these comments and personal experiences. \$\endgroup\$ – skiwi Jan 13 '15 at 16:53
  • \$\begingroup\$ No problem. Good luck! \$\endgroup\$ – Eric Stein Jan 13 '15 at 16:59
  • \$\begingroup\$ I've just posted my follow-up, in case you are interested: codereview.stackexchange.com/questions/78763/… \$\endgroup\$ – skiwi Jan 27 '15 at 15:30
3
\$\begingroup\$

NotReadableException

The NotReadableException doesn't support the Exception interface. Calling getMessage and other Exception methods won't return any data since nothing is passed up in the constructor. Consider these three constructors from NoMoreInputException. They pass useful data up the hierarchy.

public NoMoreInputException(final String message) {
    super(message);
}

public NoMoreInputException(final String message, final Throwable cause) {
    super(message, cause);
}

public NoMoreInputException(final Throwable cause) {
    super(cause);
}
\$\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.