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

CLDR Ticket #9830(accepted survey)

Opened 21 months ago

Last modified 6 weeks ago

Cleanup for Ticket 6301

Reported by: mark Owned by: backend
Component: survey Data Locale:
Phase: dsub Review:
Weeks: Data Xpath:
Xref:

ticket:6301

Description (last modified by mark) (diff)

This review is coming in too late for v30, so these are for v31.

  1. Don't add a time field to Counter. That class is used in a bunch of other places. Instead, create a subclass that adds the value (CounterWithTime?), and use that subclass in VoteResolver.
  1. Date looks like it is being used in an odd way, being set in the following, and then passed around in a bunch of revised APIs.

...

Date date = new Date();

...

int maxcounter = 100;

...

Date date = new Date(++maxcounter);

It appears to not always be a real date, which looks fragile.

Better to define a new class (TimeStamp?) that wraps a long. Also needs more comments to explain what is going on.

  1. Long sequences of commented-out code with /*...*/ are hard to follow. Especially with cases like:

436 else{ */
437 /* for (T item : items.keySet()) {

Use // (there's a command in Eclipse for that).

  1. I see

nameTime.put(info.getName(), time.getTime());

What if two info values have the same name? Looks like the time will be overwritten with the last one added (which may not be the last one chronologically.

  1. General comment: when adding overloaded methods, it is better to vector through one method with all the parameters. Makes the code more robust.

That is, given:

700 public void add(T value, int voter, Integer withVotes, Date date) {
701 if (resolved) {
702 throw new IllegalArgumentException("Must be called after clear, and before any getters.");
703 }
704 organizationToValueAndVote.add(value, voter, withVotes, date);
705 values.add(value);
706 }

You have the old method as follows, which you changed by adding the new Date line.

public void add(T value, int voter, Integer withVotes) {

598 718 if (resolved) {
599 719 throw new IllegalArgumentException("Must be called after clear, and before any getters.");
600 720 }

721 Date date = new Date();
722 organizationToValueAndVote.add(value, voter, withVotes, date);

602 723 values.add(value);

Instead, change to:

public void add(T value, int voter, Integer withVotes) {

add(value, voter, withVotes, new Date());

}

Attachments

Change History

comment:1 Changed 21 months ago by mark

  • Description modified (diff)

comment:2 Changed 21 months ago by mark

  • Xref set to 6301

comment:3 Changed 18 months ago by pedberg

  • Owner changed from anybody to parth
  • Status changed from new to accepted
  • Type changed from unknown to survey
  • Component changed from unknown to survey
  • Milestone changed from UNSCH to 31

comment:4 Changed 17 months ago by emmons

  • Milestone changed from 31 to 32

comment:5 Changed 15 months ago by fredrik

  • Owner changed from parth to mark

PM BRB: The PMs are unsure of user impact tool. Can you clarify so we can assess priority?

comment:6 Changed 14 months ago by mark

  • Priority changed from assess to minor

comment:7 Changed 9 months ago by mark

  • Priority changed from minor to medium
  • Milestone changed from 32 to 33

comment:8 Changed 4 months ago by mark

  • Milestone changed from 33 to 34

comment:9 Changed 7 weeks ago by mark

More robust voting behavior. Particularly worried about the overwriting.

comment:10 Changed 6 weeks ago by mark

  • Owner changed from mark to backend
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.