7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Shouldn't Packet initialize String type? \$\endgroup\$ – Solomon Ucko Mar 16 '18 at 21:25
  • \$\begingroup\$ Gson does not require a constructor. However, I do usually provide a constructor for the purposes of serializing packets. I will add one. \$\endgroup\$ – Daniel Causebrook Mar 16 '18 at 21:52
2
\$\begingroup\$

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 static final 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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very much for this review, it teaches me a lot! Just wanted to explain points 3, 9 and 14: These apparently unnecessary things are used for when I need an ID, timestamp or other meta info in the Packet class, and not in the Data class. However, I should have said this in the question! \$\endgroup\$ – Daniel Causebrook May 2 '18 at 16:36
3
\$\begingroup\$

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 :)

\$\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.