On Wed, 6 Aug 2025 20:25:38 GMT, David Beaumont <d...@openjdk.org> wrote:
>>> "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). I think it's OK in the current form you have. Having this class `abstract` would have forced the implementing classes to be sure about what they want to return from these methods, but as you note, this is an internal interface (implemented by very specific/few classes) and you have added the method javadocs to them. So it's OK in its current form. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26054#discussion_r2266752232
