This is the mail archive of the mailing list for the Java project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] RuleBasedCollator

Tom Tromey wrote:

"Guilhem" == Guilhem Lavaux <> 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.


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]