RFR: JDK-8293776 : Adds CSS 4 and 8 digits hex coded Color [v6]

Alexey Ivanov aivanov at openjdk.org
Mon Sep 26 11:07:24 UTC 2022


On Fri, 23 Sep 2022 22:25:09 GMT, ScientificWare <duke at openjdk.org> wrote:

> Could we reach a conciliation with this version. ? All cases are treated in 2 or 3 tests.

Your new versions introduce one more condition to each case and make the code more complicated and less clear. I am for concise, clear and readable code. I am against premature optimisation, and it hasn't been proved yet that there's a performance issue.

Combining the cases of `n == 3` and `n == 4` avoids code duplication by introducing one more comparison to both cases. If this becomes a critical path, JIT could optimise the code.

As such, I am still for combining these two cases. If you insist, I'm okay with your original code with code duplication. Yet I'd like to eliminate the duplicate code.

> Some articles suggest to avoid using exceptions as flow controls. I never tested this point. That's why I introduced Patterns in this method.

It's true, creating an exception object is a somewhat expensive operation. Yet you say, “_most_ of color notations are correct”, therefore an incorrect color which cannot be parsed is rare, so it's _an exceptional situation_, it is okay to use exceptions in this case. You also say, “Pattern usage has a significant time cost,” at the same time, you introduce a small time penalty for each and every correct color.

I think the hex pattern is redundant.

> I also wish to test the version with bit rotation too :
> 
> ```java
>             return new Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),8),
>                             true);
> ```

It could be not as clear, yet it works.

May I suggest wrapping the code for `Color` and `rotateRight` arguments?


            return new Color(
                    Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),
                                        8),
                    true);


It fits better into 80-column limit. Alternatively, wrap the second parameter of `rotateRight` only:

            return new Color(Integer.rotateRight(Integer.parseUnsignedInt(digits, 16),
                                                 8),
                             true);

This way it's easier to see how many bits are rotated.

-------------

PR: https://git.openjdk.org/jdk/pull/10317



More information about the client-libs-dev mailing list