Querz / NBT

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

Move deserialization to static method or separate classes #24

Closed Marcono1234 closed 4 years ago

Marcono1234 commented 5 years ago

It might be good to move the deserialization to static methods or have separate classes (can be nested) such as TagDeserializer (base), StringTagDeserializer, etc.. Though I think using static methods and registering them using method references might be better.

The problem with the current design is that it is not consistent (e.g. ListTag might or might not replace the existing value) and probably irritating to the user.

Let me know what you think about this.

Querz commented 5 years ago

I think the serialisation and deserialisation methods should remain in the respective tag classes, because that's the most logical place they could be. You're right about ListTag and CompoundTag being inconsistent though, during deserialisation they should not alter their value but completely reset it to whatever deserialisation was reading instead of adding to the existing value.

Marcono1234 commented 5 years ago

What about making them static though? Or do you think that there are cases where someone wants to override the contents of a tag using this?

Querz commented 5 years ago

The entry point for deserialisation is already static. Whether having it inside the Tag class or in some Factory class doesn't really matter, i just think it's more intuitive to have it inside the Tag class. Moving everything to a different class would in my opinion also defy the point of OOP to have methods acting on the fields inside of the object. Being able to override these methods is also a reason why i would like to keep it that way. People still won't be able to override the default nbt tags though, because they would need to register the child classes as custom tags but in TagFactory it's not allowed to override existing tags. They would need to register the child class with a different id.

Marcono1234 commented 5 years ago

You are right, having separate classes for it is not really worth it. I mentioned them because I thought they would make it easier due to inheritance, however it appears that this is not needed. The TagFactory could store the deserializers instead of the tag factories.

Querz commented 5 years ago

Maybe then having multiple factories for different kinds of deserialisation and serialisation (currently toString, toTagString and the regular nbt serialisation and deserialisation) would make sense? So you could register the tags serialisation and deserialisation methods there and they would do the rest. I'll think about this.

Querz commented 4 years ago

Abstracted all serialization and deserialization.