On Wed, 7 Aug 2024 16:03:59 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Whether a style converter supports reconstruction should be a statement
>> about the type, not a statement about the instance. For this reason, I don't
>> prefer an instance method like `isSupportReconstruction()`. In any case,
>> these are our options:
>> 1. Boolean parameter / Feature enum passed to constructor
>> 2. Instance method `isSupportReconstruction()`
>> 3. Annotation
>> 4. Empty marker interface
>> 5. Interface with the `convertBack` method
>> 6. Subclassing
>>
>> I lean slightly towards the annotation, but I'm generally okay with either
>> option.
>
> I just saw a boolean variable being instantiated from an annotation and
> thought "why jump through the multiple hoops?". since there is a boolean,
> why not pass it directly?
>
> it's less about memory allocation (though I would prefer to minimize that as
> well, but as @hjohn pointed out the difference is just a few bytes), but more
> about "entities must not be multiplied beyond necessity".
Although I'm not advocating for a specific change, I think looking at how `T
convert(Map)` works when it was newly introduced in Java 9 should be taken into
account as well.
There seem to be a couple of general approaches:
1. Have a marker that can be checked from the outside
- Annotation (checked from the outside)
- Unusual for this kind of check
- Marker interface
- Seen more often, but unusual to not put the new method there
- Subtype
- Bad idea, you can only inherit once
2. Introduce a new interface with the new method
- Defines the method and serves as the marker at the same time
3. Have a method that can be called that guards a 2nd method call
- Doesn't matter how this is fed (constructor, annotation, override)
- You see this more often, but it's not a great pattern (2 method
calls for the price of one...)
4. Return a special value from a method that may be unsupported
- The most obvious (modern) candidate is to return `Optional` but is a bit
unusual to indicate support/non-support
- Throw `UnsupportedOperationException` -- although standard, I think it
indicates a programmer mistake that should be avoided (in other words,
encountering this exception in production code should result in a code fix)
- Return `null`
Now the very last option (returning `null`) was the way this was handled when
`T convert(Map)` was introduced in Java 9. Even though it may not be the
prettiest solution, it has two things going for it:
- It's fast (probably the fastest of the above options, although for most by
just a slim margin)
- It fits in with the existing design
So, my order of preference:
- Just return `null`
- A new interface, which includes the new method
- A marker only interface
- A supportsXYZ method (regardless of how that is approached)
I don't think I could get behind any of the other options, because they either
stick out like a sore thumb versus the existing design, limit future options or
just perform too poorly.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1522#discussion_r1707899228
