On Tue, 9 Jul 2024 17:32:29 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> I missed something in my above evaluation. The counterpart method
>> `writeBinary` is not `static`. Although that makes sense as in that case
>> you do have an instance you wish to write, it does make the read/write API
>> asymmetric.
>>
>> It's not possible to make `readBinary` an instance method though as it is
>> the method that is creating those instances.
>>
>> The other way around is possible (make `writeBinary` static), but it would
>> then again need a pattern match to determine the correct static
>> `writeBinary` to call when writing an arbitrary `Selector`. However, this
>> would have allowed `CompoundSelector` to call a static version of
>> `SimpleSelector#writeBinary` and `readBinary`, avoiding the extra
>> identifying byte in the binary format, and avoiding the cast when reading it
>> back.
>>
>> The read/write loops below:
>>
>> +
>> final int nSelectors = is.readShort();
>> final List<SimpleSelector> selectors = new ArrayList<>();
>> for (int n=0; n<nSelectors; n++) {
>> selectors.add((SimpleSelector)Selector.readBinary(bssVersion,
>> is,strings));
>> }
>> +
>> os.writeShort(selectors.size()); // writes the number of
>> SimpleSelectors to the binary stream
>> for (int n=0; n< selectors.size(); n++)
>> selectors.get(n).writeBinary(os,stringStore); // writes each individually
>>
>> Would then have become:
>>
>> +
>> final int nSelectors = is.readShort();
>> final List<SimpleSelector> selectors = new ArrayList<>();
>> for (int n=0; n<nSelectors; n++) {
>> selectors.add(SimpleSelector.readBinary(bssVersion,
>> is,strings)); // cast is gone
>> }
>> +
>> os.writeShort(selectors.size()); // writes the number of
>> SimpleSelectors to the binary stream
>> for (int n=0; n< selectors.size(); n++)
>> SimpleSelector.writeBinaryStatic(selectors.get(n), os, stringStore); //
>> writes each individually
>
> it looks to me readBinary should be static, and writeBinary an instance
> method - this is a normal pattern. the asymmetry is dictated by the fact
> that we don't have an instance when reading, so typically read() methods read
> the stream and create an instance at the last moment with the specific
> constructor. unless, of course, the design allows for mutation and the
> fields can be set().
>
> Alternatively, readBinary() could be moved to another class (helper? reader?)
> to avoid making this an API change.
>
> what do you think?
I can see if I can externalize this or if that would run into issues. Also
please note, although technically an API change, it is NOT an accessible API
(and so can be removed at any time) because only the permitted types can access
this API.
In other words, this could wait and be done separately or never.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1333#discussion_r1670949974
