On Tue, 30 Sep 2025 18:05:16 GMT, Damon Nguyen <[email protected]> wrote:
>> Code review https://git.openjdk.org/jdk/pull/27197 for >> [JDK-8367348](https://bugs.openjdk.org/browse/JDK-8367348) made me think how >> to avoid adding more parameters to methods, in particular >> `createInstructionUIPanel`. The `Builder` object captures all the required >> configuration data, it is the `Builder` object that should be used to pass >> the configuration. >> >> This changeset refactors UI creation in `PassFailJFrame`. >> >> * The remaining constructor that accepts positional parameters now creates a >> builder to pass the configuration data. >> * The `createInstructionUIPanel` method now accepts `Builder` instead of a >> set of parameters from it. >> * The `createUI` method with positional parameters has become redundant and >> is removed. Code duplication between two versions of `createUI` is now >> eliminated. >> >> There are no functional differences. I verified it by launching a few tests >> which use `PassFailJFrame` constructors and builder. > > test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1846: > >> 1844: InvocationTargetException { >> 1845: try { >> 1846: validate(); > > Do you know when `build` is invoked? I see this `validate` is removed in > favor of having `builder.validate()` on line 508. Overall, the changes look > like they're cleanly refactored but this is the biggest change I see, > although minor. I don't understand your question… Yet I'll try to answer it. `Builder.build()` is invoked as the last step of using the `Builder` class to configure and create an object of `PassFailJFrame`. The usual sequence is: PassFailJFrame.builder() .instructions(INSTRUCTIONS) .testUI(SampleManualTest::createTestUI) .build() .awaitAndCheck(); `Builder.build()` was the only method that called `PassFailJFrame(Builder)` constructor. Before the calling the constructor, `validate` was used to validate the parameters in `Builder` and throw an exception if the required parameters, such as instructions, aren't provided. Now, there are two places where `PassFailJFrame(Builder)` is called, the new one is at lines 489–493. An instance of `Builder` is created there and is passed to the `PassFailJFrame(Builder)` constructor without calling `validate()`. Since `validate` doesn't return a value (and it shouldn't), the `validate` method cannot be called in a chained sequence. No other statements are allowed in constructors before calling `this`, therefore there's no way to create a `Builder` instance and call `validate` on it before passing it to the `PassFailJFrame` constructor. (Yes, I know that other statements are allowed in JDK 26 (and 25?), but previous versions would have to come up with another way.) Validating the instance of `Builder` inside the `PassFailJFrame` constructor covers both cases, thus it ensures `Builder.validate` is always called before creating UI. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27321#discussion_r2392584725
