Refactoring the codec to be used solely with MQTT/SSE

[codemerx]

With the generic client library codecs infrastructure gone and plugins stepping in this library should be used standalone only and only in the cases where client library is not available. This should be closer to its final form in that case.

[QuintinWillison]

I've requested a review from @marto83, given he's been working on this very recently for .NET.

[marto83]

This is a general comment about the Vcdiff decoder class. It works 👍 which is great and we can make it better.
As @QuintinWillison said it will be better to avoid accepting object for applyDelta when all we care for is byte[] or base64 string. How about we make 2 methods applyDelta(byte[]) and applyDeltaBase64(string). This way people will explicitly have to call the right method. I doubt somebody will receive a mix of the 2. It will either be base64 messages on a text transport or byte[] on a binary transport.
The other method setBase also invites errors. You should be able to create an un-initialised Decoder class. Can we make the constructor private and have 3 factory methods.

• Create(byte[], (optional) baseId)
• Create(string, (optional) baseId)
• CreateFromBase64(string, (optional) baseId)
  This way we can make setBase private and it will be harder for people to unintentionally create a scenario where the decoder is invalid.

[QuintinWillison]

As @QuintinWillison said it will be better to avoid accepting object for applyDelta when all we care for is byte[] or base64 string. How about we make 2 methods applyDelta(byte[]) and applyDeltaBase64(string). This way people will explicitly have to call the right method. I doubt somebody will receive a mix of the 2. It will either be base64 messages on a text transport or byte[] on a binary transport.

My preference is still per my original comment and implement as an overload. Java has a strong type system so that approach is more idiomatic.

[codemerx]

This is a general comment about the Vcdiff decoder class. It works 👍 which is great and we can make it better.
As @QuintinWillison said it will be better to avoid accepting object for applyDelta when all we care for is byte[] or base64 string. How about we make 2 methods applyDelta(byte[]) and applyDeltaBase64(string). This way people will explicitly have to call the right method. I doubt somebody will receive a mix of the 2. It will either be base64 messages on a text transport or byte[] on a binary transport.

There might be a string message that is not base64 encoded binary.

The other method setBase also invites errors. You should be able to create an un-initialised Decoder class. Can we make the constructor private and have 3 factory methods.

* Create(byte[], (optional) baseId)
* Create(string, (optional) baseId)
* CreateFromBase64(string, (optional) baseId)
This way we can make setBase private and it will be harder for people to unintentionally create a scenario where the decoder is invalid.

That was the initial implementation. @mattheworiordan requested it to be changed to the current form. As far as I can remember that was done so that less code needs to be written, i.e. deltas are simpler to use albeit not completely fail safe. We had a long discussion about it and I do not know whether it makes sense to reopen that discussion again and flip the implementation once more.

[marto83]

There might be a string message that is not base64 encoded binary.

I thought when you are applying deltas you will only either get a base64 or a byte[].

Regarding the other comment. Moving to factory methods will add an extra check. Currently the user will only have to check if the message is a base64 encoded message. The other disadvantage is that you will have to wait to create the Decoder until the first message arrives.

[codemerx]

I completely refactored the API and made it truly generic. There was a lot of legacy coming from the old codecs infrastructure. Actually the changes I made were specified in https://github.com/ably/wiki/blob/bd7b4f44174a65f17856283c1943d621bbe0b9ea/pages/ImplementingDeltasForANewTech.md but we are keeping too many things in the air now and I lose track.

@QuintinWillison@marto83 check out the API now and let me know what you think.