Reading/writing binary structures: how to simplify this code?

Question

Hi folks, I'm writing a network app, which sends and receives a lot of different kinds of binary packets, and I'm trying to make adding new kinds of packets to my app as easy as possible.

For now, I created a Packet class, and I create subclasses of it for each different kind of packet. However, it isn't as clean as it seems; I've ended up with code like this:

static class ItemDesc extends Packet {
    public final int item_id;
    public final int desc_type;
    public final String filename;
    public final String buf;

    public ItemDesc(Type t, int item_id, int desc_type, String filename, String buf) {
        super(t); // sets type for use in packet header
        this.item_id = item_id;
        this.desc_type = desc_type;
        this.filename = filename;
        this.buf = buf;
    }

    public ItemDesc(InputStream i) throws IOException {
        super(i); // reads packet header and sets this.input
        item_id = input.readInt();
        desc_type = input.readByte();
        filename = input.readStringWithLength();
        buf = input.readStringWithLength();
    }

    public void writeTo(OutputStream o) throws IOException {
        MyOutputStream dataOutput = new MyOutputStream();
        dataOutput.writeInt(item_id);
        dataOutput.writeByte(desc_type);
        dataOutput.writeStringWithLength(filename);
        dataOutput.writeStringWithLength(buf);
        super.write(dataOutput.toByteArray(), o);
    }
}

What bothers me about this approach is the code repetition - I'm repeating the packet structure four times. I'd be glad to avoid this, but I can't see a reasonable way to simplify it.

If I was writing in Python I would create a dictionary of all possible field types, and then define new packet types like this:

ItemDesc = [('item_id', 'int'), ('desc_type', 'byte'), ...]

I suppose that I could do something similar in any functional language. However, I can't see a way to take this approach to Java.

(Maybe I'm just too pedantic, or I got used to functional programming and writing code that writes code, so I could avoid any repetition :))

Thank you in advance for any suggestions.

Answer 1

Seem okay to me. You may just want to abstract some of the 'general' parts of the packet up the inheritance chain, so you don't need to read them, but it makes sense to be repeating the format like you are, because you've got a case for reading in raw from the constructor, reading from a stream, and writing. I see nothing wrong with it.

Answer 2

I agree with @silky that your current code is a good solution. A bit of repetitious (though not duplicated) code is not a bad thing, IMO.

If you wanted a more python-like solution, you could:

1. Replace the member attributes of ItemDesc with some kind of order-preserving map structure, do the serialization using a common writeTo method that iterates over the map. You also need to add getters for each attribute, and replace all uses of the existing fields.
2. Replace the member attributes with a Properties object and use Properties serialization instead of binary writes.
3. Write a common writeTo method that uses Java reflection to access the member attributes and their types and serialize them.

But in all 3 cases, the code will be slower, more complicated and potentially more fragile than the current "ugly" code. I wouldn't do this.

Answer 3

I am not sure you can do this in java- but maybe you could reuse one of the ctors:

public ItemDesc(InputStream i) throws IOException {
    super(i); // reads packet header and sets this.input

    this(input.readInt(), input.readByte(), input.readStringWithLength(), input.readStringWithLength());
}

Were 'this' means a call to this classes ctor, whtever the syntax might be.