On Mon, 11 Mar 2024 22:09:09 GMT, Phil Race <[email protected]> wrote:
>> The `Builder` class in `PassFailJFrame` is public and has a public
>> constructor. At the same time, a better design would be to hide all the
>> Builder constructors and rely on the `builder()` method which returns an
>> instance of the `Builder` class.
>>
>> This PR makes the constructor of the `Builder` class private so that it's
>> not accessible directly.
>>
>> Use `PassFailJFrame.builder()` to get an instance of the builder and
>> configure parameters of `PassFailJFrame`.
>>
>> I updated all the tests which used the constructor directly by calling `new
>> PassFailJFrame.Builder()`.
>>
>> I converted a few tests to use the `testUI`, which greatly simplifies the
>> test code. This change also removes flickering of the test UI.
>
> test/jdk/java/awt/regtesthelpers/PassFailJFrame.java line 1070:
>
>> 1068: }
>> 1069:
>> 1070: public Builder title(String title) {
>
> So what about this one ? Are too many tests using it ?
What about them? All the methods in `Builder` return `Builder`, that's
perfectly fine.
The goal of this PR is to make impossible to write
new PassFailJFrame.Builder();
which is replaced with
PassFailJFrame.builder();
In the former case, the test explicitly depends on the fact that the builder
pattern implementation is in the `PassFailJFrame.Builder` class.
In the latter case, I can change the `builder()` method to return another
class. As long as that other class has the same set of methods, like
`instructions` and title`, which return the same object, nothing breaks.
Encapsulation.
With this change, all the existing tests continue to run. Those tests which
failed to compile because of a breaking API change are updated in this PR.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18206#discussion_r1520497593
