Parsing log files of HearthStone: The log reading API

Question

I'm still working on a parser that can parse log entries from a game called HearthStone, the overall idea is that it will read the log file live when the game is running, parses the log file and show interesting and useful data in real time.

For the question the focus is on the API I have created to parse log entries. I have written this code using Java 8 and my intention is that the API has at least surprises as possible, everything that makes you wonder might be a candidate to include in your review.

This question is a follow-up to Extensible Abstract Log Reader and is closely related to Parsing log files of HearthStone: The API.

Class summary:

• com.github.skiwi2.hearthmonitor.logreader
  • LogReader
    • Interface
    • Used to read log entries.
  • CloseableLogReader
    • Interface
    • Used to read log entries from a closeable resource.
  • DefaultLogReader
    • Class
    • Used to read log entries from a log source via an iterator.
    • It is encouraged to extend this class and pass the iterator via the subclass.
  • LogReaderUtils
    • Utility class to create a LogReader from other sources.
  • EntryParser
    • Interface
    • Used to parse lines from a log source
    • It has the option to read more lines from the log source via a line reader if deemed necessary.
  • LineReader
    • Interface
    • Used to read lines from an input source.
  • MatchingIterator
    • Interface
    • Used to iterate over elements while also having the possibility to check if the next elements matches a predicate.
  • IteratorUtils
    • Utility class to do complex things with iterators.
  • NotParsableException
    • Exception to indicate that a log entry is not parsable.
  • NotReadableException
    • Exception to indicate that a log entry is not readable.
• com.github.skiwi2.hearthmonitor.logreader.logreaders
  • FileLogReader
    • Used to read log entries from a log file.
  • ListLogReader
    • Used to read log entries from a list.
  • MonitoringFileLogReader
    • Used to read log entries from a log file, blocking until more input is available.

---

/**
 * Used to read log entries.
 *
 * @author Frank van Heeswijk
 */
public interface LogReader {
    /**
     * Returns the next log entry.
     *
     * The NotReadableException has more information available.
     * You can recover the lines that could not be read by calling NotReadableException#getLines.
     * You can see which exceptions were thrown internally by calling NotReadableException#getOccurredExceptions.
     *
     * @return  The next log entry.
     * @throws NotReadableException If the log entry could not be read.
     * @throws java.util.NoSuchElementException If there is no more input.
     */
    LogEntry readNextEntry() throws NotReadableException;

    /**
     * Returns whether there is a next log entry.
     *
     * @return  Whether there is a next log entry.
     */
    boolean hasNextEntry();
}

---

/**
 * Used to read log entries from a closeable resource.
 *
 * @author Frank van Heeswijk
 */
public interface CloseableLogReader extends LogReader, AutoCloseable { }

---

/**
 * Used to read log entries from a log source via an iterator.
 *
 * It is encouraged to extend this class and pass the iterator via the subclass.
 *
 * @author Frank van Heeswijk
 */
public class DefaultLogReader implements LogReader {
    private final Set<? extends EntryParser> entryParsers;
    private final MatchingIterator<String> matchingIterator;

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

    /**
     * Constructs a new DefaultLogReader instance.
     *
     * @param entryParsers  The set of entry parsers
     * @param readIterator  The iterator used to read lines from the log source
     * @param filterPredicate   The predicate to use to filter the lines read from the log source
     * @throws  java.lang.NullPointerException  If entryParsers, readIterator or filterPredicate is null.
     */
    protected DefaultLogReader(final Set<? extends EntryParser> entryParsers, final Iterator<String> readIterator, final Predicate<? super String> filterPredicate) {
        Objects.requireNonNull(filterPredicate, "filterPredicate");
        Objects.requireNonNull(readIterator, "readIterator");
        this.entryParsers = Objects.requireNonNull(entryParsers, "entryParsers");
        Iterator<String> filteredIterator = IteratorUtils.filteredIterator(readIterator, filterPredicate);
        this.matchingIterator = MatchingIterator.fromIterator(filteredIterator);
    }

    /**
     * Constructs a new DefaultLogReader instance.
     *
     * The filter predicate can be used to filter the lines you want to traverse.
     *
     * @param entryParsers  The set of entry parsers
     * @param readIterator  The iterator used to read lines from the log source
     * @throws  java.lang.NullPointerException  If entryParsers or readIterator is null.
     */
    protected DefaultLogReader(final Set<? extends EntryParser> entryParsers, final Iterator<String> readIterator) {
        this(entryParsers, readIterator, string -> true);
    }

    @Override
    public LogEntry readNextEntry() throws NotReadableException {
        List<Exception> occurredExceptions = new ArrayList<>();

        String line = matchingIterator.next();
        linesInMemory.add(line);
        for (EntryParser entryParser : entryParsers) {
            if (!entryParser.isParsable(line)) {
                continue;
            }
            try {
                LogEntry result = entryParser.parse(line, new LineReader() {
                    @Override
                    public String readNextLine() {
                        String nextLine = matchingIterator.next();
                        linesInMemory.add(nextLine);
                        return nextLine;
                    }

                    @Override
                    public boolean hasNextLine() {
                        return matchingIterator.hasNext();
                    }

                    @Override
                    public boolean nextLineMatches(final Predicate<? super String> condition) {
                        return matchingIterator.nextMatches(condition);
                    }
                });
                linesInMemory.clear();
                return result;
            } catch (NotParsableException | NoSuchElementException ex) {
                occurredExceptions.add(ex);
                //try next entry parser
            }
        }
        List<String> notParsableLines = new ArrayList<>(linesInMemory);
        linesInMemory.clear();
        throw new NotReadableException(notParsableLines, occurredExceptions);
    }

    @Override
    public boolean hasNextEntry() {
        return matchingIterator.hasNext();
    }
}

---

/**
 * Utility class to create a LogReader from other sources.
 *
 * @author Frank van Heeswijk
 */
public final class LogReaderUtils {
    private LogReaderUtils() {
        throw new UnsupportedOperationException();
    }

    /**
     * Creates a LogReader that can read log entries from an input string, a LineReader for the extra lines and a read condition.
     *
     * The LogReader will attempt to read extra lines from the LineReader as long as the read condition is met using the supplied entry readers.
     *
     * Note: The input will always be included and not checked against the extra read condition.
     *
     * @param input The input line
     * @param extraLineReader    The extra line reader
     * @param extraReadCondition The extra read condition
     * @param entryParsers  The set of entry parsers
     * @return  A new LogReader that can read log entries from the input string, the LineReader for the extra lines and the read condition.
     */
    public static LogReader fromInputAndExtraLineReader(final String input, final LineReader extraLineReader, final Predicate<? super String> extraReadCondition, final Set<? extends EntryParser> entryParsers) {
        Objects.requireNonNull(extraLineReader, "extraLineReader");
        Objects.requireNonNull(extraReadCondition, "extraReadCondition");
        Objects.requireNonNull(entryParsers, "entryParsers");
        return new DefaultLogReader(entryParsers, createReadIteratorForFromInputAndExtraLineReader(input, extraLineReader, extraReadCondition));
    }

    /**
     * Returns an iterator for the given input and extra line reader.
     *
     * @param input The given input
     * @param extraLineReader   The given extra line reader
     * @param extraReadCondition    The given extra read condition
     * @return  The iterator for the given input and extra line reader.
     */
    private static Iterator<String> createReadIteratorForFromInputAndExtraLineReader(final String input, final LineReader extraLineReader, final Predicate<? super String> extraReadCondition) {
        LineReader conditionalLineReader = LineReader.readWhile(extraLineReader, extraReadCondition);
        Iterator<String> lineReaderIterator = new Iterator<String>() {
            @Override
            public boolean hasNext() {
                return conditionalLineReader.hasNextLine();
            }

            @Override
            public String next() {
                return conditionalLineReader.readNextLine();
            }
        };
        return Stream.concat(
            Stream.of(input),
            StreamSupport.stream(Spliterators.spliteratorUnknownSize(lineReaderIterator, Spliterator.NONNULL), false)
        ).iterator();
    }
}

---

/**
 * Used to parse lines from a log source.
 *
 * It has the option to read more lines from the log source via a line reader if deemed necessary.
 *
 * @author Frank van Heeswijk
 */
public interface EntryParser {
    /**
     * Returns whether this entry parser can parse the input.
     *
     * @param input The input check parsability for
     * @return  Whether this entry parser 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.
     */
    LogEntry parse(final String input, final LineReader lineReader) throws NotParsableException;
}

---

/**
 * Used to read lines from an input source.
 *
 * @author Frank van Heeswijk
 */
public interface LineReader {
    /**
     * Reads the next line.
     *
     * @return  The next line.
     * @throws java.util.NoSuchElementException If there are no lines left anymore
     */
    String readNextLine() throws NoSuchElementException;

    /**
     * Returns whether there is a next line to read.
     *
     * @return  Whether there is a next line to read.
     */
    boolean hasNextLine();

    /**
     * Returns whether the next line matches the given condition.
     *
     * @param condition The condition that the next line should match
     * @return  Whether the next line matches the given condition.
     */
    boolean nextLineMatches(final Predicate<? super String> condition);

    /**
     * Returns a LineReader that reads from another LineReader while the read condition is true.
     *
     * @param lineReader The LineReader to be read from
     * @param readCondition The read condition
     * @return A LineReader that reads from another LineReader while the read condition is true.
     * @throws java.lang.NullPointerException If lineReader or readCondition is null.
     */
    static LineReader readWhile(final LineReader lineReader, final Predicate<? super String> readCondition) {
        Objects.requireNonNull(lineReader, "lineReader");
        Objects.requireNonNull(readCondition, "readCondition");
        return new LineReader() {
            @Override
            public String readNextLine() throws NoSuchElementException {
                if (!lineReader.nextLineMatches(readCondition)) {
                    throw new NoSuchElementException();
                }
                return lineReader.readNextLine();
            }

            @Override
            public boolean hasNextLine() {
                return lineReader.nextLineMatches(readCondition);
            }

            @Override
            public boolean nextLineMatches(final Predicate<? super String> condition) {
                return lineReader.nextLineMatches(line -> (readCondition.test(line) && condition.test(line)));
            }
        };
    }
}

---

/**
 * Used to iterate over elements while also having the possibility to check if the next elements matches a predicate.
 *
 * @author Frank van Heeswijk
 * @param <E>   The type of elements
 */
public interface MatchingIterator<E> extends Iterator<E> {
    /**
     * Returns whether the next element matches the given condition.
     *
     * @param condition The condition predicate that the next element may match
     * @return  Whether the next element matches the given condition.
     * @throws  java.lang.NullPointerException  If condition is null.
     */
    boolean nextMatches(final Predicate<? super E> condition);

    /**
     * Returns a matching iterator constructed from an iterator.
     *
     * @param iterator  The input iterator
     * @param <E>   The type of the elements in the iterator
     * @return  The matching iterator constructed from the iterator.
     */
    static <E> MatchingIterator<E> fromIterator(final Iterator<? extends E> iterator) {
        return new MatchingIterator<E>() {
            private final List<E> peekedElements = new ArrayList<>();

            @Override
            public boolean hasNext() {
                Optional<E> peekElement = peek();
                return peekElement.isPresent();
            }

            @Override
            public E next() {
                if (!peekedElements.isEmpty()) {
                    return peekedElements.remove(0);
                }
                return iterator.next();
            }

            @Override
            public boolean nextMatches(final Predicate<? super E> condition) {
                Objects.requireNonNull(condition, "condition");
                Optional<E> peekElement = peek();
                return (peekElement.isPresent() && condition.test(peekElement.get()));
            }

            /**
             * Returns an optional containing the next element, or an empty optional if there is none.
             *
             * @return  The optional containing the next element, or the empty optional if there is none.
             */
            private Optional<E> peek() {
                if (!peekedElements.isEmpty()) {
                    return Optional.ofNullable(peekedElements.get(0));
                }
                if (!iterator.hasNext()) {
                    return Optional.empty();
                }
                E element = iterator.next();
                peekedElements.add(element);
                return Optional.ofNullable(element);
            }
        };
    }
}

---

/**
 * Utility class to do complex things with iterators.
 *
 * @author Frank van Heeswijk
 */
public final class IteratorUtils {
    private IteratorUtils() {
        throw new UnsupportedOperationException();
    }

    /**
     * Returns an iterator that is a view on the given iterator only considering elements that match the condition predicate.
     *
     * @param iterator  The input iterator
     * @param condition The condition predicate that elements have to match
     * @param <E>   The type of elements
     * @return  The iterator that is a view on the given iterator only considering elements that match the condition predicate.
     */
    public static <E> Iterator<E> filteredIterator(final Iterator<? extends E> iterator, final Predicate<? super E> condition) {
        return StreamSupport.stream(Spliterators.spliteratorUnknownSize(iterator, 0), false)
            .filter(condition)
            .iterator();
    }
}

---

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

    public NotParsableException() {

    }

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

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

    public NotParsableException(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<>();
    private final List<Exception> occurredExceptions = new ArrayList<>();

    /**
     * Constructs a new NotReadableException instance.
     *
     * @param lines The lines that were not readable
     * @param occurredExceptions    The exceptions that occurred during reading
     */
    public NotReadableException(final List<String> lines, final List<Exception> occurredExceptions) {
        Objects.requireNonNull(lines, "lines");
        Objects.requireNonNull(occurredExceptions, "occurredExceptions");
        this.lines.addAll(lines);
        this.occurredExceptions.addAll(occurredExceptions);
    }

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

    /**
     * Returns the exceptions that occurred during reading.
     *
     * @return  The exceptions that occurred during reading.
     */
    public List<Exception> getOccurredExceptions() {
        return new ArrayList<>(occurredExceptions);
    }
}

---

/**
 * Used to read log entries from a log file.
 *
 * @author Frank van Heeswijk
 */
public class FileLogReader extends DefaultLogReader implements CloseableLogReader {
    private final BufferedReader bufferedReader;

    /**
     * Constructs a new FileLogReader instance.
     *
     * @param bufferedReader    The buffered reader from which to read
     * @param entryParsers  The set of entry parsers
     * @throws  java.lang.NullPointerException  If bufferedReader or entryParsers is null.
     */
    public FileLogReader(final BufferedReader bufferedReader, final Set<? extends EntryParser> entryParsers) {
        super(entryParsers, bufferedReader.lines().iterator());
        this.bufferedReader = Objects.requireNonNull(bufferedReader, "bufferedReader");
    }

    /**
     * Constructs a new FileLogReader instance.
     *
     * The filter predicate can be used to filter the lines you want to traverse.
     *
     * @param bufferedReader    The buffered reader from which to read
     * @param entryParsers  The set of entry parsers
     * @param filterPredicate   The predicate to filter the lines with
     * @throws  java.lang.NullPointerException  If bufferedReader, filterPredicate or entryParsers is null.
     */
    public FileLogReader(final BufferedReader bufferedReader, final Set<? extends EntryParser> entryParsers, final Predicate<? super String> filterPredicate) {
        super(entryParsers, bufferedReader.lines().iterator(), filterPredicate);
        this.bufferedReader = Objects.requireNonNull(bufferedReader, "bufferedReader");
    }

    @Override
    public void close() throws IOException {
        bufferedReader.close();
    }
}

---

/**
 * Used to read log entries from a list.
 *
 * @author Frank van Heeswijk
 */
public class ListLogReader extends DefaultLogReader {
    /**
     * Constructs a new ListLogReader instance.
     *
     * This method saves a snapshot of the list at this time, and uses that to iterate over.
     *
     * @param inputList The input list to read from
     * @param entryParsers  The set of entry parsers
     * @throws  java.lang.NullPointerException  If inputList or entryParsers is null.
     */
    public ListLogReader(final List<String> inputList, final Set<? extends EntryParser> entryParsers) {
        super(entryParsers, new ArrayList<>(inputList).iterator());
    }

    /**
     * Constructs a new ListLogReader instance.
     *
     * This method saves a snapshot of the list at this time, and uses that to iterate over.
     * The filter predicate can be used to filter the lines you want to traverse.
     *
     * @param inputList The input list to read from
     * @param entryParsers  The set of entry parsers
     * @param filterPredicate   The predicate to filter the lines with
     * @throws  java.lang.NullPointerException  If inputList, filterPredicate or entryParsers is null.
     */
    public ListLogReader(final List<String> inputList, final Set<? extends EntryParser> entryParsers, final Predicate<? super String> filterPredicate) {
        super(entryParsers, new ArrayList<>(inputList).iterator(), filterPredicate);
    }
}

---

/**
 * Used to read log entries from a log file, blocking until more input is available.
 *
 * @author Frank van Heeswijk
 */
public class MonitoringFileLogReader extends DefaultLogReader implements CloseableLogReader {
    private final BufferedReader bufferedReader;

    /**
     * Constructs a new MonitoringFileLogReader instance.
     *
     * @param bufferedReader    The buffered reader from which to read
     * @param entryParsers  The set of entry parsers
     * @throws  java.lang.NullPointerException  If bufferedReader or entryParsers is null.
     */
    public MonitoringFileLogReader(final BufferedReader bufferedReader, final Set<? extends EntryParser> entryParsers) {
        super(entryParsers, createReadIterator(bufferedReader));
        this.bufferedReader = Objects.requireNonNull(bufferedReader, "bufferedReader");
    }

    /**
     * Constructs a new MonitoringFileLogReader instance.
     *
     * The filter predicate can be used to filter the lines you want to traverse.
     *
     * @param bufferedReader    The buffered reader from which to read
     * @param entryParsers  The set of entry parsers
     * @param filterPredicate   The predicate to filter the lines with
     * @throws  java.lang.NullPointerException  If bufferedReader, filterPredicate or entryParsers is null.
     */
    public MonitoringFileLogReader(final BufferedReader bufferedReader, final Set<? extends EntryParser> entryParsers, final Predicate<? super String> filterPredicate) {
        super(entryParsers, createReadIterator(bufferedReader), filterPredicate);
        this.bufferedReader = Objects.requireNonNull(bufferedReader, "bufferedReader");
    }

    /**
     * Returns an iterator for the given buffered reader.
     *
     * @param bufferedReader    The given buffered reader
     * @return  The iterator for the given buffered reader.
     */
    private static Iterator<String> createReadIterator(final BufferedReader bufferedReader) {
        Iterator<String> bufferedReaderIterator = bufferedReader.lines().iterator();
        return new Iterator<String>() {
            private boolean isInterrupted = false;

            @Override
            public boolean hasNext() {
                return !isInterrupted;
            }

            @Override
            public String next() {
                if (isInterrupted) {
                    throw new NoSuchElementException();
                }
                try {
                    while (!bufferedReaderIterator.hasNext()) {
                        Thread.sleep(100);
                    }
                    return bufferedReaderIterator.next();
                } catch (InterruptedException | UncheckedIOException ex) {
                    isInterrupted = true;
                    Thread.currentThread().isInterrupted();
                    throw new NoSuchElementException();
                }
            }
        };
    }

    @Override
    public void close() throws IOException {
        bufferedReader.close();
    }
}

This project (including its unit tests) is available on Github, example usage can also be found in the unit tests.

I'd like a review on every aspect of this code with an extra focus on the public API (I deem this stable for a next release).

Answer 1

The Design

A LogReader reads a log, filters the unwanted stuff and converts it into LogEntries. According to your design.

That's a bit much. Split it up.

Make one LogReader that just reads the file and presents you with endless strings. Oh wait, that's a BufferedReader! I'd see the LogReader basically as an iterator wrapper for BufferedReader.

Make a LogParser that takes a Iterable<String> and parses them into LogEntries. Then make a LogFilter that takes a bunch of LogEntries and returns only the ones you want.

Because the input and the output of the LogFilter are the same type, it's now optional for a caller. This simplifies your API when it comes to simple tasks.

---

The code looks good. I only have small things to point out:

isEmpty check

DefaultLogReader has a Set of EntryParser, and it's final. You even check if it's not null via Objects.requireNotNull ... but you don't check if it actually contains something.

For DefaultLogReader, that means that if you supply an empty set of EntryParsers, you'll get an NotReadableException the first time you try to read something. I'm not sure that's something you want. Given that you have made it an exception rather than a member attribute, I'm inclined to say that you should add checks to see if collections passed to you actually contain something.

Data Structures

private final List<E> peekedElements = new ArrayList<>();

@Override
public boolean hasNext() {
    Optional<E> peekElement = peek();
    return peekElement.isPresent();
}

@Override
public E next() {
    if (!peekedElements.isEmpty()) {
        return peekedElements.remove(0);
    }
    return iterator.next();
}

@Override
public boolean nextMatches(final Predicate<? super E> condition) {
    Objects.requireNonNull(condition, "condition");
    Optional<E> peekElement = peek();
    return (peekElement.isPresent() && condition.test(peekElement.get()));
}

/**
 * Returns an optional containing the next element, or an empty optional if there is none.
 *
 * @return  The optional containing the next element, or the empty optional if there is none.
 */
private Optional<E> peek() {
    if (!peekedElements.isEmpty()) {
        return Optional.ofNullable(peekedElements.get(0));
    }
    if (!iterator.hasNext()) {
        return Optional.empty();
    }
    E element = iterator.next();
    peekedElements.add(element);
    return Optional.ofNullable(element);
}

Let's see...

You add to the end and remove from the beginning. No other interactions occur with the List.

Why did you go with a List? A Queue is a lot better for this (it's designed for this!).

Unneeded member variable

DefaultLogReader has linesInMemory. This is not needed; it can be a final local in readNextEntry.

Class name / type mismatch

You have FileLogReader that takes a BufferedReader. Isn't this a BufferedReaderLogReader? How do you know it's a file? I bet I could jury rig stdin into your FileLogReader. I suggest getting an InputStream instead and making it a StreamLogReader or perhaps even taking a File or a Path... but that might not be what you want. Alter the name appropriately after making a decision.

Constants

                while (!bufferedReaderIterator.hasNext()) {
                    Thread.sleep(100);
                }

Perhaps you want to make this somewhat configurable? I'd put in a constant POLL_INTERVAL or something.

Exceptions

You make use of Objects.requireNotNull in numerous places. Just providing the variable names is helpful, but perhaps a proper message "entryParsers are null for [class]"?

Parameter order

You've purposefully made each subclass of DefaultLogReader have their first argument be the most important one... which happens to be the iterator provider. So why do you then have entryParsers as first argument for DefaultLogReader? You're messing with the order of arguments and making implementers making do a double check (hang on, it's reversed?). It's a small point, but I don't have any bigger ones.

---

But part of a public API is the documentation that goes along with it. So lets focus on that.

Redundant documentation

/**
 * Constructs a new DefaultLogReader instance.
 *
 * @param entryParsers  The set of entry parsers
 * @param readIterator  The iterator used to read lines from the log source
 * @param filterPredicate   The predicate to use to filter the lines read from the log source
 * @throws  java.lang.NullPointerException  If entryParsers, readIterator or filterPredicate is null.
 */
protected DefaultLogReader(final Set<? extends EntryParser> entryParsers, final Iterator<String> readIterator, final Predicate<? super String> filterPredicate) {
    Objects.requireNonNull(filterPredicate, "filterPredicate");
    Objects.requireNonNull(readIterator, "readIterator");
    this.entryParsers = Objects.requireNonNull(entryParsers, "entryParsers");
    Iterator<String> filteredIterator = IteratorUtils.filteredIterator(readIterator, filterPredicate);
    this.matchingIterator = MatchingIterator.fromIterator(filteredIterator);
}

What can we learn from the documentation which is not learnable from the code?

Well, suppose the implementation went missing.

/**
 * Constructs a new DefaultLogReader instance.
 *
 * @param entryParsers  The set of entry parsers
 * @param readIterator  The iterator used to read lines from the log source
 * @param filterPredicate   The predicate to use to filter the lines read from the log source
 * @throws  java.lang.NullPointerException  If entryParsers, readIterator or filterPredicate is null.
 */
protected DefaultLogReader(final Set<? extends EntryParser> entryParsers, final Iterator<String> readIterator, final Predicate<? super String> filterPredicate) {
    //whoops
}

Information provided by documentation bolded.

This method creates a new DefaultLogReader. Well yeah, that's what constructors do. entryParsers is a set of EntryParsers. That's what the code says, thanks.
readIterator is the iterator used to read lines from the log source. Ah, I see.
filterPredicate is the predicate to use to filter the lines read from the log source. Hmm. Oh, and it will throw a NullPointerException if you give null for any of them.

You should strive to provide meaningful documentation for every single thing documented - or leave it blank. In this case, the general constructor message and entryParsers do not provide meaningful information.

Interesting, for instance, in that constructor at the top there is that the entryParsers are not copied. If you later modify that set, it will have effects on the internal workings of the DefaultLogReader. Oops.

For the method description - you have two constructors! Why is this one different? Consider writing something like "Constructs a new DefaultLogReader instance with line filtering."

Javadoc Links

/**
 * Used to parse lines from a log source.
 *
 * It has the option to read more lines from the log source via a line reader if deemed necessary.
 *
 * @author Frank van Heeswijk
 */
public interface EntryParser

It has the option to read more lines from the log source via a line reader if deemed necessary.

I'd suggest placing a @link here. https://stackoverflow.com/questions/10097199/javadoc-see-or-link

Like that, you emphasize that it can use a LineReader rather than some object.

Double space

/**
 * Exception to indicate that a log entry is not readable.
 *
 * @author Frank van Heeswijk
 */
public class NotReadableException extends Exception  {

Exception {

Check your auto-formatting settings. Auto-format before posting a question on CR/committing to a repository.

Answer 2

I'll just go through this from top to bottom

    /**
     * Returns the next log entry.
     *
     * The NotReadableException has more information available.
     * You can recover the lines that could not be read by calling NotReadableException#getLines.
     * You can see which exceptions were thrown internally by calling NotReadableException#getOccurredExceptions.
     *
     * @return  The next log entry.
     * @throws NotReadableException If the log entry could not be read.
     * @throws java.util.NoSuchElementException If there is no more input.
     */
    LogEntry readNextEntry() throws NotReadableException;

Here you do something strange. You describe things about the NotReadableException in the javadoc to a method which throws that exception. The complete second paragraph of this javadoc should be on the Class-level javadoc of NotReadableException

---

public interface CloseableLogReader extends LogReader, AutoCloseable { }

This is something which seems to be a bad idea to me. I'd expect every log reader to implement AutoCloseable. Mostly because they're readers.

If you have readers that aren't AutoCloseable (or have resources to be closed) you could apply following trick:

public interface LogReader extends AutoCloseable {
    // Your code here:

    @Override
    default void close() {
        // cheat to not be forced to override
    }
}

---

 * It is encouraged to extend this class and pass the iterator via the subclass.

Your javadoc here somewhat contradicts the code. You explicitly encourage extending the class, but you still declare your instance fields private. If you'd really encourage extending that class, I'd have expected them to be protected instead!

---

            entryParser.parse(line, new LineReader() {
                @Override
                public String readNextLine() {
                    String nextLine = matchingIterator.next();
                    linesInMemory.add(nextLine);
                    return nextLine;
                }

                @Override
                public boolean hasNextLine() {
                    return matchingIterator.hasNext();
                }

                @Override
                public boolean nextLineMatches(final Predicate<? super String> condition) {
                    return matchingIterator.nextMatches(condition);
                }
            });

EEEEK! Why are you creating a new LineReader() { ... } here? That makes no sense, especially when you could just create one LineReader classwide. You're intermingling abstraction levels here.

First you hide the reader by passing it as parameter to the entryParser.parse(), and then you slap Mr. Maintainer in the face with (in comparison) low-level code. This exerts a lot of mental strain and makes your code hard to follow.

The rest? I just quickly read over it and can't say much, but another thing that bugged me:

Your lines (especially in javadocs) are very long! Almost every single block of code you present here has a horizontal scrollbar :( Just because it's javadoc, that doesn't mean it can go on and on into eternity. Even more so when the linebreaks are ignored html-style and a line more or less doesn't make a difference when viewing the javadoc in the documentation view.

Answer 3

I am not convinced that your general structure is useful as it should be. I have been trying to untangle how your code us used, and what sequence of events happen in order for the code to run the way it should.... I have run in to a number of issues in that process, and they all add up to a suggestion that you are using the wrong approach.

Even with your current code, there are the following items to add (in addition to what other reviewers have pointed out)...

Generics

The EntryParser class should be generified:

public interface EntryParser {
    boolean isParsable(final String input);
    LogEntry parse(final String input, final LineReader lineReader) throws NotParsableException;
}

That should be:

public interface EntryParser<T extends LogEntry> {
    boolean isParsable(final String input);
    T parse(final String input, final LineReader lineReader) throws NotParsableException;
}

All your EntryParsers should then be updated to have this new generic type.

Abstractions

You have far too many layers of abstraction in your code. Whenever you see 'marker interfaces', I get suspicious, and you don't only have marker interfaces, you have inherited markers?

public interface LogObject { }

and

public interface LogEntry extends LogObject { }

Like, really? What value do those add?

Oh, I see what those are...... right.

You have created those interfaces because you don't have the generics on the Parser. I get it.

You don't need them at all, you can just have:

public interface EntryParser<T> {
    boolean isParsable(final String input);
    T parse(final String input, final LineReader lineReader) throws NotParsableException;
}

Then it "just works".

Call Directions

I don't exactly know how to describe this problem I see in your system, but you are doing things backwards.

You have multiple entry parsers, and they all get polled to identify whether they can process a certain set of data. When they can parse the data, they then get given a 'reader' from which they can read more data. This is a problem because the parser should no tbe given the reader... it should instead be using a push model.

Consider a bunch of entries you want to parse from the logs. Create a single 'nexus' that is fed lines. It starts off in an 'idle' state. Then, as it gets lines, and it feeds the lines to the possible parsers, until one of them can handle the line.

If one can handle it (canParse(line) == true), it lets that parser 'own' the lines until the parseLine(line) method returns false....

public interface EntryParseListener<T> {
    public void parsedEntry(T entry);
}

public abstract EntryParser<T> {
    private final List<EventListener<T>> listeners = new ......

    public abstract boolean canParse(String line);
    public boolean parseLine(String line);

    protected void notifyListeners(T event) {
        for (EventListener<T> listener : listeners) {
            listener.parsedEntry(event);
        }
    }

    public void addListener(EventListener<T> listener) {
        ....
    }

}

Now, your code looks like:

1. get line source
2. get a line, find if any parser can handle.
3. feed lines to the parser until the event is parsed
4. when the event is parsed, notify the listeners that work is done.
5. return the parser in to a 'fishing expedition' looking for a parser.

If you want to pull an event from a listener, you can easily do so by adding a listener.

If the listener feeds events in to a (blocking) queue, you can iterate on that queue, and pull events as you need them. One thread parsing the files, and other threads waiting for the needed data.

That's the way I would do it....

Actual code

I would start off with an interface like:

@FunctionalInterface
public interface LogEventListener<T> {

    public void logEvent(T event);
    
}

When this method is called, an event of the listener's type has just been encountered in the log file.

This is managed by an abstract class, and concrete classes that look for certain events. A Concrete class that just parses single lines, would be something like:

import java.util.regex.Pattern;

import hearthstone.LogEntryParser;;

public class PowerLineParser extends LogEntryParser<String> {
    
    private static final Pattern POWER = Pattern.compile("\\s*\\[Power\\].*");

    @Override
    public boolean canParse(String line) {
        return POWER.matcher(line).matches();
    }

    @Override
    public String nextLine(String line) {
        int pos = line.indexOf("[Power]");
        // always return
        return "This is power line " + line.substring(pos + 8);
    }

    @Override
    public void close() {
        
    }

}

That is based on the underlying abstract class:

import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;

public abstract class LogEntryParser<T> {
    
    private final Set<LogEventListener<T>> listeners = new LinkedHashSet<>();
    
    public final void addListener(LogEventListener<T> listener) {
        synchronized(listeners) {
            listeners.add(listener);
        }
    }
    
    public final void removeListener(LogEventListener<T> listener) {
        synchronized(listeners) {
            listeners.remove(listener);
        }
    }
    
    /**
     * Called to determine which parser, if any, will be used to parse the next lines.
     */
    public abstract boolean canParse(String line);
    
    /**
     * Parse a line of data, Return a complete event, if it is complete, null otherwise
     * @param line the next line in the event to parse
     * @return the parsed event (if it is ready to be parsed).
     */
    public abstract T nextLine(String line);
    
    /**
     * Called in the event that no additional data will be arriving.
     */
    public abstract void close();

    private void notifyListeners(final T event) {
        List<LogEventListener<T>> tonotify = getListeners();
        for (LogEventListener<T> listener : tonotify) {
            listener.logEvent(event);
        }
    }

    public final boolean parse(String line) throws LogParseException {
        T event = nextLine(line);
        if (event == null) {
            return false;
        }
        notifyListeners(event);
        return true;
    }
    
    private final List<LogEventListener<T>> getListeners() {
        synchronized (listeners) {
            return new ArrayList<>(listeners);
        }
    }
    
}

That parser can identify lines that interest it. If the line can be parsed, it is fed back in to the parse method, and all following lines until the parse method returns true.

A simple exception is also useful. Making it a RuntimeException makes this easy tp put in to streams at some point too:

public class LogParseException extends RuntimeException {

    private static final long serialVersionUID = 1L;

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

The engine that drives this system looks like:

import java.util.LinkedHashSet;
import java.util.Set;

public final class LogProcessor {

    private final Set<LogEntryParser<?>> parsers = new LinkedHashSet<>();
    private final Object syncLock = new Object();
    private LogEntryParser<?> active = null;
    
    public void addParser(LogEntryParser<?> parser) {
        synchronized(syncLock) {
            parsers.add(parser);
        }
    }
    
    public void processLine(String line) {
        synchronized (syncLock) {
            if (active == null) {
                active = findActive(line);
            }
            if (active != null) {
                if (active.parse(line)) {
                    active = null;
                }
            }
            
        }
    }

    private LogEntryParser<?> findActive(String line) {
        for (LogEntryParser<?> p : parsers) {
            if (p.canParse(line)) {
                return p;
            }
        }
        return null;
    }
}

Finally, this all works when put through the paces like:

public static void main(String[] args) throws IOException {
    LogProcessor processor = new LogProcessor();
    PowerLineParser plp = new PowerLineParser();
    plp.addListener(System.out::println);
    processor.addParser(plp);
    
    try (BufferedReader br = Files.newBufferedReader(Paths.get(args[0]))) {
        String line = null;
        while ((line = br.readLine()) != null) {
            processor.processLine(line);
        }
    }
}