On Fri, 1 Aug 2025 13:36:48 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
>> "is completed" is a non-directory type, "getLocation() failing" is a >> non-resource type. The abstract class is consistently saying "I'm not a >> thing of /that/ type" (because it isn't and these are the only answers it >> can give). >> I don't think there's *any* risk or issue here. The abstract class is >> internal, clearly documented and overloaded exactly once by a trusted >> collaborator. Do you really think this is a "risk"? And if you do, please >> give your suggestion as to what would be better. Otherwise nothing is >> actionable here. > >> "is completed" is a non-directory type, "getLocation() failing" is a >> non-resource type. > > If getLocation() by default throws a `IllegalStateException` stating that the > `Node` is not a resource type, then I think it would be more accurate for > `isCompleted()` to by default return `false` to match the claim that the > `Node` is not a resource type. Hmm, I see the argument, but then I have to make two additional overrides to make it return true in the cases where it's always, unconditionally, true. I don't like adding more code for no functional improvement though. Remember that (unlike the current code) this change removes "isCompleted()" as a visible method, so it's only usable internally and cannot be used in unexpected ways. Outside of asserts, it's now called exactly once, so it's not hard to reason about it, however it's implemented. I mean I could remove it from Node and only have it as a Directory method and use instanceof and a cast before calling it ... but it's more code in the actual business logic and doesn't achieve anything because the chance of misuse is zero, since everything would break if it was somehow used incorrectly. Basically isComplete() == false => it's a directory that need its children populated. So, I could flip the return value and name is "isIncompleteDirectory()" which is more precise (an "uglier" name draws attention to the fact it captures something about the special case nature of directories). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2258215768
