On Fri, 26 Jun 2026 14:46:05 GMT, Andy Goryachev <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java
>> line 138:
>>
>>> 136: if (id == null) {
>>> 137: throw new IllegalArgumentException("DataFormat
>>> id must not be null.");
>>> 138: }
>>
>> Suggestion:
>
> the proposed code is no different? in any case, throwing an NPE might be
> better in this case.
I'm proposing to remove this since the `null` check in `ids` was done with
`Set.of(ids)`. It's unreachable code (unless `ids` is changed in a background
thread maybe, but I don't think this is a real concern, and you can work with
`identifier` instead anyway).
>> modules/javafx.graphics/src/main/java/javafx/scene/input/DataFormat.java
>> line 145:
>>
>>> 143: }
>>> 144: isNew = false;
>>> 145: }
>>
>> Suggestion:
>>
>> boolean noneExist = Collections.disjoint(this.identifie,
>> registry.keySet());
>
> my version is better as it avoids scanning twice
This claim might be too strong. From a performance perspective, the sizes of
the collections are very small (a `DataFormat` will realistically have at most
3 ids and the total number of ids/mime-types that exist in the wild is probably
in the dozens only), so performance is not really a consideration. However, if
we do look at it, your implementation iterates over the array, which is O(n),
with a hashed `get` which is O(1). `Collections.disjoint` is optimized to the
given collections - it iterates over the smaller collection, which is O(n), and
checks contains which is O(1). So I don't think your code gives better
performance.
>From a readability perspective, what your code says is "for each id, find a
>data format that uses it" (and if the `DataFormat` shares 2 or more ids, you
>will find and compare the same `DataFormat` twice or more). `disjoint` is
>clearer: "does any of the new ids exist already?"
In fact, I'm not sure why you're throwing only when not all the identifiers are
the same, that is, why you specified "if one of the given ids is already
assigned to another DataFormat **with a different set of ids**". If some are
the same it's an exception, but if all of them are the same then it's not?
Maybe I'm missing something.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3482768434
PR Review Comment: https://git.openjdk.org/jfx/pull/2197#discussion_r3483102546
