Querz / NBT

A java implementation of the NBT protocol, including a way to implement custom tags.
MIT License
182 stars 48 forks source link

Improve overall type safety #18

Closed Querz closed 5 years ago

Querz commented 5 years ago

See #14 ListTag should remember its type when cleared.

The current solution makes the no-args constructor of ListTag protected so deserialisation can still create an instance without knowing the type of the ListTag. Everything from outside creating a new instance of ListTag will need to use ListTag(Class<T extends Tag>).

I also modified ListTag#getTypeID() and ListTag#getTypeClass() to match the old behaviour and be consistent with serialisation. Another reason for this is that these methods cannot be consistent with each other when the ListTag is empty. Example:

ListTag<IntTag> l = new ListTag<>(IntTag.class);

can only set the type class of the ListTag, but not the type id. Setting the type id occurs when adding the first element to the new ListTag. Therefore ListTag#getTypeID() will return 0 and ListTag#getTypeClass() returns EndTag.class when the ListTag is empty.


Other cases that will fail now but worked before:

ListTag<IntTag> l = new ListTag<>(IntTag.class);
l.addByte((byte) 123); // will now throw an IllegalArgumentException
ListTag<IntTag> l = new ListTag<>(IntTag.class);
l.addInt(123);
l.clear();
l.addByte((byte) 123); // will now throw an IllegalArgumentException

The behaviour of ListTag#asTypedList(Class<? extends Tag>) also matches the new behaviour.


All Unit Tests have been adjusted.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+1.4%) to 98.962% when pulling c8214205715256c53c5c9d868fe276a6f6a3e3cd on listtag-type into 947c750bbe0a4cdce79b3bf51ec941aaf657f4ff on master.

Querz commented 5 years ago

The last commit removes the typeID from ListTag and changes the TagFactory to work with this. ListTag only uses the type class now. We can also look into using it with an Optional. I don't mind using it, i just didn't have the time for it yet.

Querz commented 5 years ago

Maybe an unchecked (and therefore empty) list should always be equal to any other empty list, but then hashCode must not include the type class anymore when the list is empty.

If not then at least an unchecked list should be equal to another unchecked list.

Behaviour now is that ListTag#equals() will return true if both ListTags are either empty AND untyped or empty and with the same type or not empty, with the same type and same values in the same order.

ListTag#hashCode() matches this behaviour by including the type of the ListTag.


Another thing i noticed which is not very important but i'd like to change is the way ListTag#toString() prints the values by using the full #toString() on each tag, which includes the type of each tag. This information is redundant as their type is already printed in the "type" field. The result of ListTag#toString() should be something like this:

{
    "type": "ListTag",
    "value": {
        "list": [
            -128,
            0,
            127
        ],
        "type": "ByteTag"
    }
}