Re: [PATCH] RuleBasedCollator

[Guilhem Lavaux]

Tom Tromey wrote:

> > > > > > "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.
>  
Thank you. ;-)

> 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.
>  
Ok.

> 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".
>  
There isn't any reason. It's simply I should have forgotten to mark it 
final again when I've merged source code.

> 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.
>  
I'll replace it with an "if" if it doesn't pose problems (it's sometimes 
difficult to find how to use try ... catch).

Thank you very much for taking the time of reviewing it.

Guilhem.