ably-flutter icon indicating copy to clipboard operation
ably-flutter copied to clipboard

lib/src/codec.dart could use some rework and tests

Open zoechi opened this issue 5 years ago • 1 comments

getCodecType should be rather _CodecPair getCodec(...) and return a codec instead of returning a number that can used to look up the _CodecPair in a Map?

The way getCodecType is written, it would fail if someone added an entry to codecMap because it would not be covered by the if/else in getCodecType anyway. So codecMap should in my opinion be private, or if getCodecType would be _CodecPair getCodec(...), codecMap would not be required at all.

I'd also try making _CodecPair const.

There are also no tests for this file.

There are probably more issues, but I didn't dig deeper.

┆Issue is synchronized with this Jira Task by Unito

zoechi avatar Sep 10 '20 07:09 zoechi

Copied from a PR comment https://github.com/ably/ably-flutter/pull/33/files#

I think I commented somewhere already, that I think the methods readFromJson writeToJson are not worth having. The assert for DateTime seems rather arbitrary, because most types can not be encoded. It would also be possible to add a toEncodeable parameter to jsonEncode with a default encoder for DateTime for example to encode it as .toUtc().toIso8601String(). That would probably need something equivalent DateTime.parse() on the decoder side.

If they stay they should at least be made private. and the explicit return type void should be added to writeToJson.

This line in readFromJson is redunant

if(value==null) return null;

(before return value as T;)

zoechi avatar Sep 10 '20 08:09 zoechi