BarDweller
commented
7 years ago Reviewed 11 of 26 files at r1. Review status: 11 of 24 files reviewed at latest revision, 12 unresolved discussions.
gojava-application/src/main/java/org/gameontext/sample/Log.java, line 63 at r1 (raw file):
// much easier to read if the default behaviour is to have this level // promotion switched on if ( level.intValue() < Level.INFO.intValue() && !NO_LOG_LEVEL_PROMOTION) {
Wondering here about stuff javac would likely optimise away, but I'd normally put the fixed constant part of the && statement first, and have the level int comparison after, as conceptually, the comparison will only need to be performed if the bool expression is false.. I'd hope a good compiler could imply that from this statement, but even so.. easy change..
gojava-application/src/main/java/org/gameontext/sample/RoomDescription.java, line 19 at r1 (raw file):
* This is how our room is described. * a) Use post-construct to go fill some of this in by asking the map * b) Assign this dynamically on the fly as the room is used.
what are a) b) here.. ? order of operation? mutually exclusive usage patterns? #confusingcomment I can't find reference to a) b) later in this file.
gojava-application/src/main/java/org/gameontext/sample/RoomDescription.java, line 94 at r1 (raw file):
} /**
There's no synchronization protection over get/add/remove, and get iterates the entryset, which is modified by add/remove .. caution needed here if these are driven in response to external events on concurrent threads.
gojava-application/src/main/java/org/gameontext/sample/RoomDescription.java, line 128 at r1 (raw file):
/** * Room inventory objects are optional. Build/cache/return a JsonArray listing * items in the room for use in location messages.
same issue re commands / concurrency
gojava-application/src/main/java/org/gameontext/sample/RoomImplementation.java, line 93 at r1 (raw file):
// Who doesn't love switch on strings in Java 8? switch(message.getTarget()) {
This feels nicer than before.. but the length of this switch statement is unfortunate.. admittedly a good 50% of that is the json commenting..
gojava-application/src/main/java/org/gameontext/sample/RoomImplementation.java, line 179 at r1 (raw file):
String contentToLower = content.toLowerCase(Locale.ENGLISH).trim(); String firstWord;
String parts[] = contentToLower.split(" ",2); //split limit.. shiny ;p String firstWord = parts[0]; String remainder = parts.length > 1 ? parts[1] : null; //array will only have 1 element if string had no spaces..
(also look at java.util.Scanner it can make token processing string input a lot simpler.. although it doesn't easily handle the 'remainder' pattern here, you could pass the Scanner instance around allowing getDirection etc to process the remaining string as required.. {can sort of emulate remainder if you add \n to the string then use scanner nextLine to glob the rest of the line.. ymmv} .. )
gojava-application/src/main/java/org/gameontext/sample/RoomImplementation.java, line 249 at r1 (raw file):
}
the direction stuff here feels.. awkward.. like direction should be some sort of construct that handles all this mapping between text and short & long for you, so it's not duplicated between the two methods (note that getDirection will return null for up / down, while long direction supports those concepts.. if that's intentional, it could do with a comment explaining why)
gojava-application/src/main/java/org/gameontext/sample/map/client/MapResponseReader.java, line 58 at r1 (raw file):
try { rdr = Json.createReader(entityStream); JsonStructure json = rdr.read();
JsonObject returnedJson = rdr.readObject(); ??
gojava-application/src/main/java/org/gameontext/sample/protocol/Message.java, line 42 at r1 (raw file):
private static final String PREFIX = "room-" + RoomImplementation.ROOM_ID + "-"; /** Incrementing message id for bookmark */
I wonder if we shouldn't init this long with say, System.currentMillis() / 100 or something.. to ensure each run of the same room generates differing bookmark ids in the ui
gojava-application/src/main/java/org/gameontext/sample/protocol/Message.java, line 71 at r1 (raw file):
/** * Ack message: this supports version 1 & 2 * {@code ack,{\"version\":[1]}}
hmm.. should this comment say 1|2 ?
gojava-application/src/main/java/org/gameontext/sample/protocol/MessageDecoder.java, line 45 at r1 (raw file):
@Override public boolean willDecode(String s) { return true;
meh.. suspect this should only return true if new Message(s) won't through a DecodeException ? not sure though.
gojava-application/src/main/java/org/gameontext/sample/protocol/MessageEncoder.java, line 40 at r1 (raw file):
@Override public String encode(Message msg) throws EncodeException { return msg.toString();
wondering if this shouldn't call a more appropriately named method to encode msg to json, I know toString is javadoc'd to say it returns json, but I kinda prefer having toString, and formatted outputters be distinct, to allow toString to be embellished for debug/log without affecting operation
Comments from Reviewable
ilanpillemer
ebullient
Signed-off-by: Erin Schnabel schnabel@us.ibm.com
This change is