ebullient
commented
8 years ago Reviewed 12 of 13 files at r1. Review status: 12 of 13 files reviewed at latest revision, 13 unresolved discussions.
gojava-application/src/main/java/application/Application.java, line 164 at r1 (raw file):
Quoted 5 lines of code…
> ``` > RoomInfo roomInfo = getRoomInfo(); > if (roomInfo != null) { > response.add(NAME, roomInfo.getName()); > String description = roomInfo.getDescription(); > response.add(DESCRIPTION, (description == null) ? "A badly described room" : description); > ``` > >Make RoomInfo a member variable, to make it easier for the room description to be reused/updated/forwarded between addNewPlayer and processCommand (below)
getRoomInfo should make sure that it always returns an object..
gojava-application/src/main/java/application/Application.java, line 201 at r1 (raw file):
response.add(NAME, roomInfo.getName()); String description = roomInfo.getDescription(); response.add(DESCRIPTION, (description == null) ? "A badly described room" : description);
This is duplicate code for above. Simplify with a reusable RoomInfo object.
Might add as a comment around that RoomInfo object that a room could consider changing its description at runtime over time, or per user, or across instances, or...
[gojava-application/src/main/java/application/Application.java, line 216 at r1](https://reviewable.io:443/reviews/gameontext/gameon-room-java/26#-KTtBYvY57UD18uOnwa:-KTtBYvtrtwOLE-Mrpd:b-74biko) (raw file):
} if ( exitDirection == null) {
You do still need to make sure that all exits are defined. They should be, but accidents happen.
gojava-application/src/main/java/map/client/Log.java, line 27 at r1 (raw file):
*/ public class Log { private final static Logger log = Logger.getLogger("net.wasdev.gameon.mediator");
Bad package here...
gojava-application/src/main/java/map/client/Log.java, line 57 at r1 (raw file):
* @return Original Level or INFO level, whichever is greater */ private static Level useLevel(Level level) {
We might use a variable here to determine whether or not this takes effect. Should be possible to not do this level promotion in some environments.
_gojava-application/src/main/java/map/client/MapClient.java, line 51 at r1 (raw file):_
public class MapClient { private static final String mapLocation = "https://game-on.org/map/v1/sites";
Should still allow this to be injected (and cope if it isn't). We have the possibility on the horizon for additional domains for test/staging... would be nice for the example to handle that.
gojava-application/src/main/java/map/client/MapResponseReader.java, line 31 at r1 (raw file):
@Provider @Consumes(MediaType.APPLICATION_JSON) public class MapResponseReader implements MessageBodyReader<Site> {
We need this because we aren't using Jackson?
gojava-application/src/main/java/map/client/MapResponseReader.java, line 58 at r1 (raw file):
site.setType(sanitiseNull(returnedJson.getJsonString("type"))); site.setId(sanitiseNull(returnedJson.getJsonString("_id")));
Why do we need to sanitize null? (because we aren't using Jackson that automatically ignores null attributes?) (especially given that we are using Jackson w/ annotations below?)
gojava-application/src/main/java/map/client/model/Doors.java, line 20 at r1 (raw file):
import com.fasterxml.jackson.annotation.JsonIgnoreProperties; @JsonIgnoreProperties(ignoreUnknown = true)
Also need @JsonInclude(Include.NON_EMPTY) .. though we won't be marshalling these back out in the usual course of things
gojava-application/src/main/java/map/client/model/Exit.java, line 17 at r1 (raw file):
*******************************************************************************/ package map.client.model;
Comments. Jackson annotations for avoiding empty values and ignoring unknown attributes
_gojava-application/src/main/java/map/client/model/Exits.java, line 22 at r1 (raw file):_
import javax.json.JsonObjectBuilder; public class Exits {
JAckson annotations for ignoring nulls and unknown attributes
_gojava-application/src/main/java/map/client/model/RoomInfo.java, line 18 at r1 (raw file):_
package map.client.model; public class RoomInfo {
Jackson annotations for ignoring unknown and empty attributes Should include assigning dummy strings for missing descriptive values
gojava-application/src/main/java/map/client/model/Site.java, line 17 at r1 (raw file):
*******************************************************************************/ package map.client.model;
Jackson annotations for ignoring unknown/empty attributes.
Comments from Reviewable
pavittr
Fixes #14
This change is