On Tue, 16 Aug 2022 00:26:47 GMT, SWinxy <[email protected]> wrote: >> ScientificWare has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Adds to CSS.java the missing color names. >> >> Adds to CSS.java, the missing color names defined by : >> CSS Color Module Level 4 >> W3C Candidate Recommendation Snapshot, 5 July 2022 >> [7.1 Named Colors](https://www.w3.org/TR/css-color-4/#named-color) >> - Updates, Copyright year. >> - Adds relative imports. >> - Replaces, if ... then ... else statements with a Map called "colorNamed". >> Code is more readable and easy to maintain. >> After tests, TreeMap seems slower than Map. Results are available at >> scientificware#12 (comment). >> >> Warning : The Previous JDK CSS Orange Color is different from CSS Color >> Recommendation. > > Hey there. The current implementation creates a new `Color` object for each > invocation of `stringToColor` (with the exception of `""` because we want to > keep developers on their toes). Using a `Map` will *not* create new `Color` > objects for each invocation, which may explain why your results show `Map` as > the most performant. This is *technically* a change in behavior, and > *technically* not wanted. > > To get around this, you can do `new Color(c.getRed(), c.getGreen(), > c.getBlue())` from the map, or--what I would prefer--to use an enhanced > switch statement to create the colors. > > One thing I'd request is to have `stringToColor` return in the branches, > rather than setting the placeholder `Color color;` object. Things like this > irk me. As in (using the map): > > static Color stringToColor(String str) { > if (str == null) { > return null; > } else if (str.length() == 0) { > return Color.black; > } else if (str.startsWith("rgb(")) { > return parseRGB(str); > } else if (str.startsWith("rgba(")) { > return parseRGBA(str); > } else if (str.charAt(0) == '#') { > return hexToColor(str); > } else if (colorNamed.containsKey(str.toLowerCase(Locale.ROOT))) { > return colorNamed.get(str.toLowerCase(Locale.ROOT)); > } else { > return hexToColor(str); > } > } > > > In general, good PR. I'd be interested to know the perf results when the > behavior is unchanged, and if the enhanced switch would win out.
@SWinxy Thanks for comments. After reading them, I realize the Big Mistake ! I Shouldn't create a new object but rather return an existing one. That was in my intention. All the code is already available [here](https://github.com/scientificware/jdk/issues/12), I wrote it because I expected to move this logic to Color.java. All color declarations are ready. The Map will be something like : static TreeMap<String, Color> colorNamed = new TreeMap<String, Color>( Map.ofEntries( Map.entry("aliceblue", ALICE_BLUE), ... Map.entry("yellowgreen", YELLOW_GREEN) ); Do you validate this approach ? ------------- PR: https://git.openjdk.org/jdk/pull/9825
