[Unicode]   Common Locale Data Repository : Bug Tracking Home | Site Map | Search
 
Modify

CLDR Ticket #7064(accepted tools)

Opened 3 years ago

Last modified 15 months ago

Problems with CheckCLDR.Options [only branch]

Reported by: mark Owned by: anybody
Component: unknown Data Locale:
Phase: dsub Review:
Weeks: Data Xpath:
Xref:

Description

In review of CLDR Ticket #6841, I found some problems (this slipped through the cracks; I'd asked for discussion before the ticket was marked for review).

Notably:

It looks like the Options class should just contain an EnumMap, instead of the more elaborate structure.

There's then a lot of redundant code, like

private void set(String key, String value) {

376 TODO- cache the map
377 for(Option o : Option.values()) {
378 if(o.getKey().equals(key)) {
379 set(o, value);
380 return;
381 }
382 }
383 throw new IllegalArgumentException("Unknown CLDR option: '" + key + "' - valid keys are: " + Options.getValidKeys());
384 }

which then can just be

map.set(Option.valueOf(key), value);

Why does it need to be Comparable?

It saves a key for 'quick comparison'. But if these are singletons (as advertised*), that isn't necessary or the best approach for simple comparison, which can be just ==.

  • > I'd like to replace the Map with a singleton Object so it can be a cache key.

And so on.

Attachments

Change History

comment:1 Changed 3 years ago by emmons

  • Status changed from new to assigned
  • Component changed from unknown to tools
  • Priority changed from assess to medium
  • Milestone changed from UNSCH to 26rc
  • Owner changed from anybody to ribnitz
  • Type changed from unknown to defect

comment:2 Changed 3 years ago by ribnitz

  • Status changed from assigned to reviewing
  • Review set to mark

comment:3 Changed 3 years ago by emmons

  • Status changed from reviewing to accepted
  • Review mark deleted

comment:4 Changed 3 years ago by ribnitz

  • Status changed from accepted to reviewing
  • Review set to mark

comment:5 Changed 3 years ago by mark

  • Status changed from reviewing to accepted

Review.

Normally a bad idea to iterate through names instead of using a hashmap, but in this case there are so few options. But add a comment that if the enum grows, to change the implementation.

options.putAll(options2.options.clone());

I don't think you need to clone. Please check this.

446 Iterator<Option> iter=options.clone().keySet().iterator();

why are you doing this?

while (iter.hasNext()) {

this is generally bad form. Use instead

for (X entry : Y.entrySet()) {
...

much faster.

comment:6 Changed 3 years ago by mark

  • Owner changed from ribnitz to googler

comment:7 Changed 3 years ago by mark

  • Owner changed from googler to anybody
  • Milestone changed from 26rc to 27dsub

comment:8 Changed 3 years ago by markus

  • Phase set to dsub
  • Milestone changed from 27dsub to 27

comment:9 Changed 2 years ago by emmons

  • Milestone changed from 27 to UNSCH

Moving all 27+anybody tickets to UNSCH.

comment:10 Changed 2 years ago by markus

  • Type changed from defect to tools
  • Component changed from tools to unknown

comment:11 Changed 15 months ago by mark

  • Review mark deleted
  • Summary changed from Problems with CheckCLDR.Options to Problems with CheckCLDR.Options [only branch]
View

Add a comment

Modify Ticket

Action
as accepted
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.