TCP Packet design

Question

I'm implementing a client/server app using JSON (as String) via TCP/IP.

String --> Packet --> byte[] --> tcp/ip --> byte[] --> Packet --> String

The String will be put into a Packet and sent via TCP/IP.

This way I can simply send a Packet by getting its raw byte[] and send it into the TCP/IP socket. On the other side I can simply create a new Packet by putting the received byte[] into a Packet again.

I'm asking you if the Packet class looks okay and if there is anything else wrong with it.

public class Packet {

    private final String content;
    private final byte[] raw;

    private static final int LENGTH_OFFSET = 2;

    public Packet (byte[] data) {
        if (data == null) {
            throw new IllegalArgumentException("data may not be null");
        }
        if (data.length < 2) {
            throw new IllegalArgumentException("invalid data lenght (must be at least two byte)");
        }
        byte[] lengthByte = new byte[2];
        System.arraycopy(data, 0, lengthByte, 0, LENGTH_OFFSET);
        int contentLength = getLength(lengthByte);
        if (contentLength > data.length-LENGTH_OFFSET) {
            throw new IllegalArgumentException("corrupted data - length > data");
        }
        raw = new byte[contentLength];
        System.arraycopy(data, LENGTH_OFFSET, raw, 0, contentLength);
        content = new String(raw);
    }

    public Packet (String cnt) {
        if (cnt == null) {
            throw new IllegalArgumentException("content may not be null");
        }
        if (cnt.length() < 0) {
            throw new IllegalArgumentException("content must not be empty");
        }       
        content = cnt;
        int contentLength = content.length();
        byte[] lengthByte = getLength(contentLength);
        byte[] stringData = content.getBytes();
        raw = new byte[contentLength + LENGTH_OFFSET];

        System.arraycopy(lengthByte, 0, raw, 0, LENGTH_OFFSET);
        System.arraycopy(stringData, 0, raw, LENGTH_OFFSET, contentLength);     
    }

    private static byte[] getLength(int length) {
        if (length > 0xFFFF) {
            throw new IllegalArgumentException("data is too long");
        }
        byte high = trimSignum((length & 0xFF00) >> 8);
        byte low = trimSignum((length & 0xFF));     
        return new byte[] {high, low};
    }

    private static byte trimSignum(int byteWithSignum) {
        if (byteWithSignum > 127) {
            return (byte)(0xFF - byteWithSignum);
        }
        return (byte)byteWithSignum;
    }

    public String getContent() {
        return content;
    }

    public byte[] getRaw() {
        return raw;
    }

    private static int getLength(byte[] lb) {
        int high = (lb[0] & 0xFF) << 8;
        int low = (lb[1] & 0xFF);       
        return high + low;
    }

}

Answer 1

Naming

• LENGTH_OFFSET sounds like the offset at which you find the length, not the offset after the length. I'd cede that I can't think of a better name though, that doesn't sound silly...

• Personally I'd prefer lengthBytes to lengthByte, since there are more than one (is this a relic of previously using 8bit lengths?).

• getLength sounds like an accessor: really this is doing a translation/conversion, and the name should reflect that.

• cnt isn't very meaningful, and your ArgumentExceptions all refer to content.

String Encoding

I would be worried about using String.getBytes and String(byte[]), as you are not explicitly indicating the encoding. I'm not a Java programmer, but judging by this answer on StackOverflow and the documentation, you should really be using overloads which allow you to specifying the encoding (e.g. UTF8), which will enable reliable communication between machines, and between implementations of the protocol written in different languages.

Misc

• raw is the whole (length + content) packet when you using the Packet(String cnt) constructor, but doesn't include the length when you use the Packet(byte[] data) constructor.

• Spelling error in exception message: "lenght"

• Nice to see lots of guard clauses: would checking for negative lengths be sensible?

• A few more empty lines would be appreciated to break up logical parts of the larger methods (e.g. in the byte[] constructor, first run some checks (empty line) then read the length (empty line) then read the content).