11
\$\begingroup\$

I am 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 this question I focus on the module that provides an API for the actual log entries, and to be more specific I focus on an entry called CreateGameLogEntry. The API is written in Java 6 in order to be compatible with Android.

Class summary:

  • LogObject
    • Marker interface to denote that a class can be used as an log object.
    • A log object can appear anywhere in a log file.
  • LogEntry
    • Marker interface to denote that a class can be used as an log entry.
    • A log entry always ranges over at least one full line of the log file, and thus is also a log object.
  • Objects
    • Utility class used to mimic JDK 7+ Objects behaviour.
  • CreateGameLogEntry
    • Entry for [Power] CREATE_GAME.
    • Uses the builder pattern.
  • CreateGameLogEntry.GameEntityLogEntry
    • Entry for [Power] CREATE_GAME - GameEntity.
    • Uses the builder pattern.
  • CreateGameLogEntry.PlayerLogEntry
    • Entry for [Power] CREATE_GAME - Player
    • Uses the builder pattern.
  • CreateGameLogEntry.PlayerLogEntry.GameAccountId
    • Object for [Power] CREATE_GAME - Player - GameAccountId
    • Uses the builder pattern.

/**
 * Marker interface to denote that a class can be used as a log object.
 *
 * A log object can appear anywhere in a log file.
 *
 * @author Frank van Heeswijk
 */
public interface LogObject { }

/**
 * Marker interface to denote that a class can be used as an log entry.
 *
 * A log entry always ranges over at least one full line of the log file, and thus is also a log object.
 *
 * @author Frank van Heeswijk
 */
public interface LogEntry extends LogObject { }

/**
 * Utility class used to mimic JDK 7+ Objects behaviour.
 *
 * @author Frank van Heeswijk
 */
final class Objects {
    private Objects() {
        throw new UnsupportedOperationException();
    }

    /**
     * Tests whether the given object is null.
     *
     * @param object    The given object
     * @param message   The message of the NullPointerException if object is null
     * @param <T>   The type of object
     * @return  The object if it is not null.
     * @throws  java.lang.NullPointerException  If object is null.
     */
    static <T> T requireNonNull(final T object, final String message) {
        if (object == null) {
            throw new NullPointerException(message);
        }
        return object;
    }
}

/**
 * Entry for [Power] CREATE_GAME.
 *
 * @author Frank van Heeswijk
 */
public class CreateGameLogEntry implements LogEntry {
    /*
     * Example
     *
     * [Power] GameState.DebugPrintPower() - CREATE_GAME
     * [Power] GameState.DebugPrintPower() -     GameEntity EntityID=1
     * [Power] GameState.DebugPrintPower() -         tag=10 value=85
     * [Power] GameState.DebugPrintPower() -         tag=TURN value=1
     * [Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
     * [Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=1
     * [Power] GameState.DebugPrintPower() -         tag=NEXT_STEP value=BEGIN_MULLIGAN
     * [Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=GAME
     * [Power] GameState.DebugPrintPower() -         tag=STATE value=RUNNING
     * [Power] GameState.DebugPrintPower() -     Player EntityID=2 PlayerID=1 GameAccountId=[hi=144115198130930503 lo=27162067]
     * [Power] GameState.DebugPrintPower() -         tag=TIMEOUT value=75
     * [Power] GameState.DebugPrintPower() -         tag=PLAYSTATE value=PLAYING
     * [Power] GameState.DebugPrintPower() -         tag=CURRENT_PLAYER value=1
     * [Power] GameState.DebugPrintPower() -         tag=FIRST_PLAYER value=1
     * [Power] GameState.DebugPrintPower() -         tag=HERO_ENTITY value=4
     * [Power] GameState.DebugPrintPower() -         tag=MAXHANDSIZE value=10
     * [Power] GameState.DebugPrintPower() -         tag=STARTHANDSIZE value=4
     * [Power] GameState.DebugPrintPower() -         tag=PLAYER_ID value=1
     * [Power] GameState.DebugPrintPower() -         tag=TEAM_ID value=1
     * [Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
     * [Power] GameState.DebugPrintPower() -         tag=CONTROLLER value=1
     * [Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=2
     * [Power] GameState.DebugPrintPower() -         tag=MAXRESOURCES value=10
     * [Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=PLAYER
     * [Power] GameState.DebugPrintPower() -         tag=NUM_TURNS_LEFT value=1
     * [Power] GameState.DebugPrintPower() -     Player EntityID=3 PlayerID=2 GameAccountId=[hi=144115198130930503 lo=37543301]
     * [Power] GameState.DebugPrintPower() -         tag=TIMEOUT value=75
     * [Power] GameState.DebugPrintPower() -         tag=PLAYSTATE value=PLAYING
     * [Power] GameState.DebugPrintPower() -         tag=HERO_ENTITY value=36
     * [Power] GameState.DebugPrintPower() -         tag=MAXHANDSIZE value=10
     * [Power] GameState.DebugPrintPower() -         tag=STARTHANDSIZE value=4
     * [Power] GameState.DebugPrintPower() -         tag=PLAYER_ID value=2
     * [Power] GameState.DebugPrintPower() -         tag=TEAM_ID value=2
     * [Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
     * [Power] GameState.DebugPrintPower() -         tag=CONTROLLER value=2
     * [Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=3
     * [Power] GameState.DebugPrintPower() -         tag=MAXRESOURCES value=10
     * [Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=PLAYER
     * [Power] GameState.DebugPrintPower() -         tag=NUM_TURNS_LEFT value=1
     */

    private final GameEntityLogEntry gameEntityLogEntry;
    private final Set<PlayerLogEntry> playerLogEntries;

    private CreateGameLogEntry(final Builder builder) {
        this.gameEntityLogEntry = Objects.requireNonNull(builder.gameEntityLogEntry, "builder.gameEntityLogEntry");
        this.playerLogEntries = Objects.requireNonNull(builder.playerLogEntries, "builder.playerLogEntries");
    }

    public GameEntityLogEntry getGameEntityLogEntry() {
        return gameEntityLogEntry;
    }

    public Set<PlayerLogEntry> getPlayerLogEntries() {
        return new HashSet<PlayerLogEntry>(playerLogEntries);
    }

    public static class Builder {
        private GameEntityLogEntry gameEntityLogEntry;
        private Set<PlayerLogEntry> playerLogEntries = new HashSet<PlayerLogEntry>();

        public Builder gameEntityLogEntry(final GameEntityLogEntry gameEntityLogEntry) {
            this.gameEntityLogEntry = Objects.requireNonNull(gameEntityLogEntry, "gameEntityLogEntry");
            return this;
        }

        public Builder addPlayerLogEntry(final PlayerLogEntry playerLogEntry) {
            Objects.requireNonNull(playerLogEntry, "playerLogEntry");
            this.playerLogEntries.add(playerLogEntry);
            return this;
        }

        public CreateGameLogEntry build() {
            return new CreateGameLogEntry(this);
        }
    }

    /**
     * Entry for [Power] CREATE_GAME - GameEntity.
     */
    public static class GameEntityLogEntry implements LogEntry {
        /*
         * Example
         *
         * [Power] GameState.DebugPrintPower() -     GameEntity EntityID=1
         * [Power] GameState.DebugPrintPower() -         tag=10 value=85
         * [Power] GameState.DebugPrintPower() -         tag=TURN value=1
         * [Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
         * [Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=1
         * [Power] GameState.DebugPrintPower() -         tag=NEXT_STEP value=BEGIN_MULLIGAN
         * [Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=GAME
         * [Power] GameState.DebugPrintPower() -         tag=STATE value=RUNNING
         */

        private final String entityId;
        private final Map<String, String> tagValues;

        private GameEntityLogEntry(final Builder builder) {
            this.entityId = Objects.requireNonNull(builder.entityId, "builder.entityId");
            this.tagValues = Objects.requireNonNull(builder.tagValues, "builder.tagValues");
        }

        public String getEntityId() {
            return entityId;
        }

        public String getTagValue(final String tag) {
            return tagValues.get(tag);
        }

        public Map<String, String> getTagValues() {
            return new HashMap<String, String>(tagValues);
        }

        public static class Builder {
            private String entityId;
            private Map<String, String> tagValues = new HashMap<String, String>();

            public Builder entityId(final String entityId) {
                this.entityId = Objects.requireNonNull(entityId, "entityId");
                return this;
            }

            public Builder addTagValuePair(final String tag, final String value) {
                Objects.requireNonNull(tag, "tag");
                Objects.requireNonNull(value, "value");
                tagValues.put(tag, value);
                return this;
            }

            public GameEntityLogEntry build() {
                return new GameEntityLogEntry(this);
            }
        }
    }

    /**
     * Entry for [Power] CREATE_GAME - Player.
     */
    public static class PlayerLogEntry implements LogEntry {
        /*
         * [Power] GameState.DebugPrintPower() -     Player EntityID=2 PlayerID=1 GameAccountId=[hi=144115198130930503 lo=27162067]
         * [Power] GameState.DebugPrintPower() -         tag=TIMEOUT value=75
         * [Power] GameState.DebugPrintPower() -         tag=PLAYSTATE value=PLAYING
         * [Power] GameState.DebugPrintPower() -         tag=CURRENT_PLAYER value=1
         * [Power] GameState.DebugPrintPower() -         tag=FIRST_PLAYER value=1
         * [Power] GameState.DebugPrintPower() -         tag=HERO_ENTITY value=4
         * [Power] GameState.DebugPrintPower() -         tag=MAXHANDSIZE value=10
         * [Power] GameState.DebugPrintPower() -         tag=STARTHANDSIZE value=4
         * [Power] GameState.DebugPrintPower() -         tag=PLAYER_ID value=1
         * [Power] GameState.DebugPrintPower() -         tag=TEAM_ID value=1
         * [Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
         * [Power] GameState.DebugPrintPower() -         tag=CONTROLLER value=1
         * [Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=2
         * [Power] GameState.DebugPrintPower() -         tag=MAXRESOURCES value=10
         * [Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=PLAYER
         * [Power] GameState.DebugPrintPower() -         tag=NUM_TURNS_LEFT value=1
         */

        private final String entityId;
        private final String playerId;
        private final GameAccountId gameAccountId;
        private final Map<String, String> tagValues;

        private PlayerLogEntry(final Builder builder) {
            this.entityId = Objects.requireNonNull(builder.entityId, "builder.entityId");
            this.playerId = Objects.requireNonNull(builder.playerId, "builder.playerId");
            this.gameAccountId = Objects.requireNonNull(builder.gameAccountId, "builder.gameAccountId");
            this.tagValues = Objects.requireNonNull(builder.tagValues, "builder.tagValues");
        }

        public String getEntityId() {
            return entityId;
        }

        public String getPlayerId() {
            return playerId;
        }

        public GameAccountId getGameAccountId() {
            return gameAccountId;
        }

        public String getTagValue(final String tag) {
            return tagValues.get(tag);
        }

        public Map<String, String> getTagValues() {
            return new HashMap<String, String>(tagValues);
        }

        public static class Builder {
            private String entityId;
            private String playerId;
            private GameAccountId gameAccountId;
            private Map<String, String> tagValues = new HashMap<String, String>();

            public Builder entityId(final String entityId) {
                this.entityId = Objects.requireNonNull(entityId, "entityId");
                return this;
            }

            public Builder playerId(final String playerId) {
                this.playerId = Objects.requireNonNull(playerId, "playerId");
                return this;
            }

            public Builder gameAccountId(final GameAccountId gameAccountId) {
                this.gameAccountId = Objects.requireNonNull(gameAccountId, "gameAccountId");
                return this;
            }

            public Builder addTagValuePair(final String tag, final String value) {
                Objects.requireNonNull(tag, "tag");
                Objects.requireNonNull(value, "value");
                tagValues.put(tag, value);
                return this;
            }

            public PlayerLogEntry build() {
                return new PlayerLogEntry(this);
            }
        }

        /**
         * Object for [Power] CREATE_GAME - Player - GameAccountId.
         */
        public static class GameAccountId implements LogObject {
            private final String hi;
            private final String lo;

            private GameAccountId(final Builder builder) {
                this.hi = Objects.requireNonNull(builder.hi, "builder.hi");
                this.lo = Objects.requireNonNull(builder.lo, "builder.lo");
            }

            public String getHi() {
                return hi;
            }

            public String getLo() {
                return lo;
            }

            public static class Builder {
                private String hi;
                private String lo;

                public Builder hi(final String hi) {
                    this.hi = Objects.requireNonNull(hi, "hi");
                    return this;
                }

                public Builder lo(final String lo) {
                    this.lo = Objects.requireNonNull(lo, "lo");
                    return this;
                }

                public GameAccountId build() {
                    return new GameAccountId(this);
                }
            }
        }
    }
}

Example usage:

GameEntityLogEntry gameEntityLogEntry = new GameEntityLogEntry.Builder()
    .entityId("1")
    .addTagValuePair("test", "random")
    .addTagValuePair("test2", "random2")
    .build();

GameAccountId gameAccountId1 = new GameAccountId.Builder()
    .hi("2")
    .lo("1")
    .build();

PlayerLogEntry playerLogEntry1 = new PlayerLogEntry.Builder()
    .playerId("1")
    .entityId("2")
    .gameAccountId(gameAccountId1)
    .addTagValuePair("test", "random")
    .addTagValuePair("test2", "random2")
    .build();

GameAccountId gameAccountId2 = new GameAccountId.Builder()
    .hi("4")
    .lo("3")
    .build();

PlayerLogEntry playerLogEntry2 = new PlayerLogEntry.Builder()
    .playerId("2")
    .entityId("3")
    .gameAccountId(gameAccountId2)
    .addTagValuePair("test", "random")
    .addTagValuePair("test2", "random2")
    .build();

CreateGameLogEntry createGameLogEntry = new CreateGameLogEntry.Builder()
    .gameEntityLogEntry(gameEntityLogEntry)
    .addPlayerLogEntry(playerLogEntry1)
    .addPlayerLogEntry(playerLogEntry2)
    .build();

I'd like to have comments on all aspects of my code.

The full log entry (for reference):

[Power] GameState.DebugPrintPower() - CREATE_GAME
[Power] GameState.DebugPrintPower() -     GameEntity EntityID=1
[Power] GameState.DebugPrintPower() -         tag=10 value=85
[Power] GameState.DebugPrintPower() -         tag=TURN value=1
[Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
[Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=1
[Power] GameState.DebugPrintPower() -         tag=NEXT_STEP value=BEGIN_MULLIGAN
[Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=GAME
[Power] GameState.DebugPrintPower() -         tag=STATE value=RUNNING
[Power] GameState.DebugPrintPower() -     Player EntityID=2 PlayerID=1 GameAccountId=[hi=144115198130930503 lo=27162067]
[Power] GameState.DebugPrintPower() -         tag=TIMEOUT value=75
[Power] GameState.DebugPrintPower() -         tag=PLAYSTATE value=PLAYING
[Power] GameState.DebugPrintPower() -         tag=CURRENT_PLAYER value=1
[Power] GameState.DebugPrintPower() -         tag=FIRST_PLAYER value=1
[Power] GameState.DebugPrintPower() -         tag=HERO_ENTITY value=4
[Power] GameState.DebugPrintPower() -         tag=MAXHANDSIZE value=10
[Power] GameState.DebugPrintPower() -         tag=STARTHANDSIZE value=4
[Power] GameState.DebugPrintPower() -         tag=PLAYER_ID value=1
[Power] GameState.DebugPrintPower() -         tag=TEAM_ID value=1
[Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
[Power] GameState.DebugPrintPower() -         tag=CONTROLLER value=1
[Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=2
[Power] GameState.DebugPrintPower() -         tag=MAXRESOURCES value=10
[Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=PLAYER
[Power] GameState.DebugPrintPower() -         tag=NUM_TURNS_LEFT value=1
[Power] GameState.DebugPrintPower() -     Player EntityID=3 PlayerID=2 GameAccountId=[hi=144115198130930503 lo=37543301]
[Power] GameState.DebugPrintPower() -         tag=TIMEOUT value=75
[Power] GameState.DebugPrintPower() -         tag=PLAYSTATE value=PLAYING
[Power] GameState.DebugPrintPower() -         tag=HERO_ENTITY value=36
[Power] GameState.DebugPrintPower() -         tag=MAXHANDSIZE value=10
[Power] GameState.DebugPrintPower() -         tag=STARTHANDSIZE value=4
[Power] GameState.DebugPrintPower() -         tag=PLAYER_ID value=2
[Power] GameState.DebugPrintPower() -         tag=TEAM_ID value=2
[Power] GameState.DebugPrintPower() -         tag=ZONE value=PLAY
[Power] GameState.DebugPrintPower() -         tag=CONTROLLER value=2
[Power] GameState.DebugPrintPower() -         tag=ENTITY_ID value=3
[Power] GameState.DebugPrintPower() -         tag=MAXRESOURCES value=10
[Power] GameState.DebugPrintPower() -         tag=CARDTYPE value=PLAYER
[Power] GameState.DebugPrintPower() -         tag=NUM_TURNS_LEFT value=1

This project (including its unit tests) is available at Github.

\$\endgroup\$
0
3
\$\begingroup\$

Overuse of null checks and abuse of the Builder pattern

You're overusing Objects.requireNonNull and abusing the Builder pattern at multiple places.

    private final GameEntityLogEntry gameEntityLogEntry;
    private final Set<PlayerLogEntry> playerLogEntries;

    private CreateGameLogEntry(final Builder builder) {
        this.gameEntityLogEntry = Objects.requireNonNull(builder.gameEntityLogEntry, "builder.gameEntityLogEntry");
        this.playerLogEntries = Objects.requireNonNull(builder.playerLogEntries, "builder.playerLogEntries");
    }

playerLogEntries in the Builder is never null. Since Builder is an inner class, under your full control, the extra null check is either paranoia, or you wrote that in auto-pilot mode :-)

When you build a CreateGameLogEntry using CreateGameLogEntry.Builder, if you forget to set gameEntityLogEntry the code will compile. That's not great, because a CreateGameLogEntry without a GameEntityLogEntry doesn't make sense, and sure enough, thanks to the null check in the constructor, the problem will manifest in an NPE at runtime. Problems showing up at runtime are not great when you can prevent them at compile time. And you can, by making GameEntityLogEntry a constructor parameter in the Builder.

If GameEntityLogEntry is never missing, a user might still willfully set a null value explicitly, but would that really be a problem? If not, then you can also remove the Objects.requireNonNull check, further reducing boilerplate code.

Adding constructor parameters to a Builder might seem like it defeats the purpose. But it doesn't. It's a (useful!) variation on the classic Builder pattern, a twist, if you will.

Extra caution about the Builder pattern

Keep in mind that the Builder pattern has some overhead: it's quite tedious to write up. It's most useful when you have 4+ optional parameters. Under 4 parameters, it's often easier to use classic constructors.

With mandatory arguments the pattern becomes an unwieldy mix. Because, the only way to ensure integrity of the class at compile time is to move those parameters into the Builder's constructor. This goes against the pattern, weakening its usefulness.

About defensive getters

In your defensive getters like this one:

    public Set<PlayerLogEntry> getPlayerLogEntries() {
        return new HashSet<PlayerLogEntry>(playerLogEntries);
    }

An interesting alternative can be to return an unmodifiable set instead of creating a new instance:

    public Set<PlayerLogEntry> getPlayerLogEntries() {
        return Collections.unmodifiableSet(playerLogEntries);
    }
\$\endgroup\$
1
  • \$\begingroup\$ While it's definitely better to use unmodifiableSet for clarity, it does create a new instance - a wrapper set that passes through some methods and throws exceptions for others. \$\endgroup\$
    – Eric Stein
    Jan 16 '15 at 16:14
2
\$\begingroup\$

Overall, this looks good. I actually like your Objects implementation to steal a bit of Java 7's magic, since the code still needs to be compiled under Java 6.

The only minor suggestion I can make is to inline your Objects.requireNonNull() inside your addTagValuePair() methods for both GameEntityLogEntry.Builder and PlayerLogEntry.Builder:

    public Builder addTagValuePair(final String tag, final String value) {
        tagValues.put(Objects.requireNonNull(tag, "tag"),
                Objects.requireNonNull(value, "value"));
        return this;
    }

And even minor point is to decide if you want a static method to create new Builder instances...

Compare

new CreateGameLogEntry.Builder()...

And

public static Builder builder() {
    return new CreateGameLogEntry.Builder();
}

CreateGameLogEntry.builder()...
\$\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.