[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] RuleBasedCollator
From: |
Tom Tromey |
Subject: |
Re: [PATCH] RuleBasedCollator |
Date: |
18 Dec 2003 10:07:55 -0700 |
User-agent: |
Gnus/5.09 (Gnus v5.9.0) Emacs/21.3.50 |
>>>>> "Guilhem" == Guilhem Lavaux <address@hidden> writes:
Guilhem> Please review.
I read through this. The code looks fine to me, based on what I
remember of RuleBasedCollator. I'm glad to see that someone is paying
attention to java.text -- this is some of our oldest and
least-worked-on code, it really deserves a face-lift.
There were a few formatting issues: lines past 79 columns that should
be wrapped, and lack of whitespace around some operators, eg:
Guilhem> for (i=0;i<prefix.length() && i < s.length();i++)
You need spaces around "=", "<", and after ";" here.
I also had a couple other questions.
Guilhem> class CollationElement
You changed this from a final class, but I didn't see anything derived
from it. Any reason why? I find marking helpers like this "final"
nice because it is a way to say "don't worry about subclasses if you
change this".
Guilhem> // Hehehe. What we would do not to use if.
Guilhem> try
Guilhem> {
Guilhem> ord1 = ord1block.getValue();
Guilhem> }
Guilhem> catch (NullPointerException _)
Guilhem> {
Guilhem> if (ord2block == null)
Guilhem> return 0;
Guilhem> return -1;
Guilhem> }
I'd prefer a simple "if" as I find it clearer to read.
Tom