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

CLDR Ticket #9830(accepted survey)

Opened 8 months ago

Last modified 5 weeks ago

Cleanup for Ticket 6301

Reported by: mark Owned by: mark
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 8 months ago by mark

  • Description modified (diff)

comment:2 Changed 8 months ago by mark

  • Xref set to 6301

comment:3 Changed 5 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 4 months ago by emmons

  • Milestone changed from 31 to 32

comment:5 Changed 7 weeks 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 5 weeks ago by mark

  • Priority changed from assess to minor
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.