[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] NumberFormat
From: |
Dalibor Topic |
Subject: |
Re: [PATCH] NumberFormat |
Date: |
Sat, 22 Nov 2003 00:11:53 +0100 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.4) Gecko/20030624 |
Hi Guilhem,
Guilhem Lavaux wrote:
Hi,
Here is just a patch to review about NumberFormat.Field before I check
it in. It implements java.text.NumberFormat.Field it is quite
straightforward using the documentation.
Looks good to me in general, I have small remarks before you check it in:
Index: java/text/NumberFormat.java
===================================================================
RCS file: /cvsroot/classpath/classpath/java/text/NumberFormat.java,v
retrieving revision 1.4
diff -u -r1.4 NumberFormat.java
--- java/text/NumberFormat.java 22 Jan 2002 22:27:01 -0000 1.4
+++ java/text/NumberFormat.java 21 Nov 2003 19:52:44 -0000
@@ -79,6 +79,98 @@
[snip]
+ private static final NumberFormat.Field[] allFields =
+ {
+ INTEGER, FRACTION, EXPONENT, DECIMAL_SEPARATOR, SIGN,
+ GROUPING_SEPARATOR, EXPONENT_SYMBOL, PERCENT,
+ PERMILLE, CURRENCY, EXPONENT_SIGN
+ };
I don't know what GNU Classpath's policy on JavaDoc comments for private
members [1] is, but as Michael Koch is cheering for checkStyle on #gcj
I assume it will become the patch submission standard, and it pretty
much wants to see comments on everything, if I recall my last CheckStyle
usage attempts.
So, could you add a javadoc comment describing the field, please?
+ // For deserialization purpose
+ private Field()
+ {
+ super("");
+ }
Same as above.
Could you add a javadoc comment for the constructor, please?
+
+ private Field(String s)
+ {
+ super(s);
+ }
This constructor is protected in the API spec for 1.4.2.
Again, a small comment would be nice.
+ protected Object readResolve() throws InvalidObjectException
+ {
+ String s = getName();
+ for (int i=0;i<allFields.length;i++)
+ if (s.equals(allFields[i].getName()))
+ return allFields[i];
+
+ throw new InvalidObjectException("no such NumberFormat field called " +
s);
+ }
+ }
Looks good to me. could you add another comment here?
Finally, the mean question: do we have mauve tests for those? If not,
would you write some, please, so that gcj devs don't go ahead again and
re-implement everything from scratch as they tend to do ;)? [2]
cheers,
dalibor topic,
who is eager to write his own patch to the java/text/FieldPosition.java
(equals) method's comment as soon as he's finished playing virtual mjw ;)
[1] I devote this pun to ricky clarckson. Mark, how are his
documentation patches coming along?