Parsing and creating JSON packets

Question

For several of my pet projects, I have had to interpret and/or send JSON WebSocket or HTTP responses in Java. I did some research and picked the library GSON to use. In each project, I have implemented a custom deserializer in order to correctly interpret different packet types.

For example:

{"type": "set-name", "data":{"name":"daniel"}} {"type": "set-timeperiod", "data":{"start":"2017-03-16", "end":"2017-03-17"}} 

In this case I would use two classes, SetName and SetTimePeriod:

public class SetName implements Data { private String name; @Override public void handle(Foo foo) throws InvalidPacketException { if(name == null) throw new InvalidPacketException(); foo.doThingWithName(name); } @Override public String getType() { return "set-name"; } } 

Foo represents any arbitrary resources, the packet might need to perform its function. The data interface is fairly straightforward:

public interface Data { void handle(Foo foo); String getType(); } 

The Data objects are wrapped in a Packet representing the whole thing:

public void Packet { private String type; private Data data; public Packet(Data data) { this.data = data; type = data.getType(); } public void handle(Foo foo) throws InvalidPacketException { data.handle(foo); } public Packet initData(Data data) { if(this.data != null) throw new IllegalStateException("Data already initialised."); this.data = data; return this; } } 

And finally, the packet would be converted from raw JSON to an instance of Packet using a custom Deserializer.

public class PacketDeserializer implements JsonDeserializer<Packet> { @Override public Packet deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException { JsonObject jsonObject = json.getAsJsonObject(); // Remove data object, GSON won't know what to turn it into. JsonObject dataObject = jsonObject.remove("data").getAsJsonObject(); // Determine what type of data object we need. Class dataClass = typeToClass(jsonObject.get("type").getAsString()); // Construct and return a Packet of the correct type. Data data = context.deserialize(dataObject, dataClass); return new Gson().fromJson(jsonObject, Packet.class).initData(data); } private Class<? extends Data> typeToClass(String type) throws JsonParseException { switch(type) { case("set-name"): return SetName.class; case("set-timeperiod"): return SetTimePeriod.class; default: throw new JsonParseException("Type " + type + " is invalid."); } } } 

This means GSON will now deserialize the object into a Packet and then call the handle() to do whatever needs to be done with that packet:

Packet packet = new GsonBuilder() .registerTypeAdapter(Packet.class, new PacketDeserializer()) .create() .fromJson(message, Packet.class); packet.handle(foo); 

In addition, a Packet instance can be converted to JSON without the need for any custom serializer. Once this structure is built, it is very easy to add new packet types or to send/parse packets.

This seems like an elegant solution to me, but I have some concerns:

• Is it actually efficient? Would it be much faster to parse the JSON directly?
• I can end up with a lot of subclasses of Data which makes me feel like there could be a better way of doing it
• If this is a good solution, would it be a good thing to use wherever I'm parsing JSON with some object with varying contents like data?

I came up with this solution years ago, when I was fairly new to programming and haven't changed it much at all, so I feel like I must be doing something wrong.

Answer 1

I have a few comments:

• Data transfer objects, that Gson works with, fields may be final and hold nulls by default (with some other remarks to primitive types that require a kind of cheating at javac).
• These Data Transfer Objects may also have private constructors, because Gson does not need any. This and the previous items are just less error-prone (you can neither instantiate such an incoming DTO manually nor change its state).
• You don't need Packet.initData.
• SetName and SetTimePeriod do similar things, and can be refactored to have a common super class that implements the template method pattern.
• I have a feeling that messsage is a string. Prefer stream-oriented classes like InputStream, Reader and JsonReader in order to avoid unnecessary buffering (not a very suitable for your case, because you still have JsonElement (that's a buffer per se) in your deserializer) -- this will work just fine for bigger objects.
• Also consider extracting Gson instance simply to staticfinal fields. Gson is immutable and thread-safe, therefore it can be easily shared across multiple-threads.
• PacketDeserializer holds no state and be made a singleton without any negative impacts. Consider hiding constructors as much as possible too.
• Having that in mind, consider returning the most super-type as you can: public static JsonDeserializer<Packet> get() { ... }. This would allow you not bother for concrete types used at call-site.
• Modifying arguments is a bad practice making a lot of surprises to anyone not expecting such a behavior: jsonObject.remove("data") (additionally this is just unnecessary and makes no effect for you).
• You should not create Gson instances in serializers and deserializers and do all such stuff via contexts only. First of all, Gson instances are relatively expensive to instantiate. Additionally, such a Gson object may have totally different configuration, other that the "global" Gson instance has. However, I suspect that you made this choice in order to avoid infinite recursion.
• PacketDeserializer.typeToClass can be static.
• switches with ( and )cases look unusual.
• By the way, string switches are totally fine for such kind of mapping.
• Packet does not need type.

As the result:

final class InvalidPacketException extends IOException { } 

interface Data { void handle(Foo foo) throws InvalidPacketException; String getType(); } 

abstract class AbstractData implements Data { private transient String type; protected AbstractData(final String type) { this.type = type; } protected abstract boolean isValid(); protected abstract void doHandle(Foo foo); @Override public final void handle(final Foo foo) throws InvalidPacketException { if ( !isValid() ) { throw new InvalidPacketException(); } doHandle(foo); } @Override public final String getType() { return type; } } 

final class SetName extends AbstractData { private final String name = null; private SetName() { super("set-name"); } @Override protected boolean isValid() { return name != null; } @Override protected void doHandle(final Foo foo) { foo.doThingWithName(name); } } 

final class SetTimePeriod extends AbstractData { private final Date start = null; private final Date end = null; private SetTimePeriod() { super("set-timeperiod"); } @Override protected boolean isValid() { return start != null && end != null; } @Override protected void doHandle(final Foo foo) { foo.doThingWithStartAndEnd(start, end); } } 

final class Packet { private final Data data; private Packet(final Data data) { this.data = data; } static Packet of(final Data data) { return new Packet(data); } void handle(final Foo foo) throws InvalidPacketException { data.handle(foo); } } 

final class PacketDeserializer implements JsonDeserializer<Packet> { private static final JsonDeserializer<Packet> instance = new PacketDeserializer(); private PacketDeserializer() { } static JsonDeserializer<Packet> get() { return instance; } @Override public Packet deserialize(final JsonElement jsonElement, final Type type, final JsonDeserializationContext context) throws JsonParseException { final JsonObject jsonObject = jsonElement.getAsJsonObject(); final JsonElement dataJsonElement = jsonObject.get("data"); final Type dataType = resolveType(jsonObject.get("type").getAsString()); final Data data = context.deserialize(dataJsonElement, dataType); return Packet.of(data); } private static Type resolveType(final String type) throws JsonParseException { switch ( type ) { case "set-name": return SetName.class; case "set-timeperiod": return SetTimePeriod.class; default: throw new JsonParseException("Type " + type + " is invalid."); } } } 

final class Foo { void doThingWithName(final String name) { System.out.println("NAME: " + name); } void doThingWithStartAndEnd(final Date start, final Date end) { System.out.println("START: " + start + "; END:" + end); } } 

private static final Gson gson = new GsonBuilder() .registerTypeAdapter(Packet.class, PacketDeserializer.get()) .create(); public static void main(final String... args) throws IOException { final Foo foo = new Foo(); for ( final String name : ImmutableList.of("name.json", "timeperiod.json") ) { // I'm just reading these as JsonReader-ed resources from the package the Q189789 class is nested at try ( final JsonReader jsonReader = Resources.getPackageResourceJsonReader(Q189789.class, name) ) { final Packet packet = gson.fromJson(jsonReader, Packet.class); packet.handle(foo); } } } 

Example output:

NAME: daniel START: Thu Mar 16 00:00:00 EET 2017; END:Fri Mar 17 00:00:00 EET 2017 

Gson also has a very convenient type adapter for your case. Note that this is an example and the type adapter is not bundled with Gson. You might want just to include it into your codebase.

Answer 2

A quick braindump:

• PacketDeserializer#deserialize is overcommented. Most comments restate what the code says. Try commenting as little as possible, but as much as necessary.
• PacketDeserializer could benefit from not hardcoding the mapping between the type strings and classes. Consider passing a Map<String, Class<? extends Data>> as constructor parameter and replacing typeToClass with a map.get
• Lastly you should also avoid rawtypes. Class without generics specification is a rawtype.

Continuing that thought, you really want a different name than Foo for the "PacketHandler" you pass in to Packet#handle. You should consider going so far as to extract the responsibility of handling the package into a separate class. A Packet should not know how to handle itself. It's originally just a data-holder, nothing intelligent :)