On Fri, 28 Feb 2025 18:06:28 GMT, Alexey Ivanov <[email protected]> wrote:
>> src/java.desktop/share/classes/com/sun/beans/finder/MethodFinder.java line
>> 81:
>>
>>> 79: try {
>>> 80: Method method = CACHE.get(signature);
>>> 81: return (method != null) ? method : CACHE.create(signature);
>>
>> This can be simplified to the `return CACHE.get(signature)`, since we know
>> that the implementation creates the value in the get method:
>>
>> https://github.com/openjdk/jdk/blob/d6c4be672f6348f8ed985416ed90d0447f5d5bb3/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L120-L126
>>
>> However, I am not a fan of this solution as it is implementation dependent
>> and this behavior is not documented.
>
> I think this is confusing… If `CACHE.get(signature)` always returns a
> non-null value, there's no need for the condition `method != null` — it's
> always true.
>
> The javadoc for the `Cache` class specifies a `null` value can be returned:
>
> https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L98-L99
>
> Is it a bug in `Cache` class that it creates the value and adds it to the
> cache when the value isn't found?
>
> https://github.com/openjdk/jdk/blob/e98df71d9c5120fbb73a4c2f49863775fe5db781/src/java.desktop/share/classes/com/sun/beans/util/Cache.java#L126-L127
Sorry for my last premature comment.
I have looked at `Cache`, and believe `create` should be made protected to
indicate it is not intended to be called, but only overridden.
The preexisting code called `create` to reuse the creation mechanism without
going through the cache - such usage is dubious. Per my experience, similar
conditional caches are better implemented by enclosing the logic into the cache
structure and use a common endpoint to access, so we have one universal site to
determine the conditionality of caching. In this case, such a condition is
better included in the `Cache` class itself, and the `create` method should not
be publicly exposed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/23845#discussion_r1975870725
