On 18/10/2012 03:10, Xueming Shen wrote:
:
webrev:
http://cr.openjdk.java.net/~sherman/4235519/webrev
I took another pass over this, focusing on the API as that is what we
have to get right. Performance is important too but I think the priority
has to be the API first.
Overall I think it is quite nice and easy to use.
I wonder if the Base64 class description should try to establish the
terms "base64" and "base64url" so that they can be referenced from the
various methods? That would avoid needing to duplicate references to the
RFC 4668 and RFC 2045 in so many places.
I think it would also be useful if the specification indicated whether
Encoders and Decoders are safe for use by concurrent threads. Also make
it clear that NPE is thrown if any of the parameters are passed as null
(unless otherwise specified of course).
I'm not sure that getUrlEncoder is the most suitable name to get a
base64url encoder. The reason is that the method name makes it sound
like it returns a URLEncoder or or at least an encoder for HTML forms.
While more verbose, getBase64UrlEncoder is clear that it returns a
base64url encoder.
Do you think getEncoder(int,byte[]) will be used much? If not then
perhaps it should be left out for the first version (I guess part of
this is getting used to providing the line separate as a byte array).
For the javadoc then Encoder and Decoder will need @since 1.8. They
should probably cross reference each other too.
encode(byte[]) should be clearer that it encodes all bytes in the given
array. Also I think it needs to be clear that the returned byte array is
appropriately sized -- as currently worded it doesn't make it clear that
they can't be extra elements at the end (odd as it might be).
Typo at line 215, "byter array" -> "byte array"
Typo at line 246, "methold" -> "method".
Typo at line 247, "arry" -> "array"
Type at line 254, "encocde" -> "encode"
Typo at line 277, "buffter" -> "buffer". Another one at line 702.
Minor comment, but I assume you should move the
@SuppressWarnings("deprecation") on encodeToString to after the method
comment rather than before. I see the same thing in decode(String).
I think encode(ByteBuffer) needs to be clear as to the
position/limit/capacity of the returned buffer.
I'm not sure so about encode(ba, null) returning the required length, it
feels odd and a bit like some of the win32 APIs. If the intention is
that the caller allocates the byte[] and then calls encode again then it
would be easier to just encode(ba).
For the decoder methods then IllegalArgumentException may be thrown
mid-flight, meaning that some bytes may have been written into output
buffer or array even though an exception is thrown. I think this needs
to be made clear in the spec. It also makes me wonder if this is the
right exception, it feels like a more specialized malformed input exception.
Another point about the decode methods is that they stop at the padding
bytes and so this allows for bytes after the padding. I'm curious about
this choice and whether you considered this as an error? I see how this
influences decode(ByteBuffer,ByteBuffer) to return a boolean and I just
wonder if there are other choices to consider.
That's mostly it for now. I didn't check in the IDE but there are lot of
imports that are don't appear to be used, perhaps left over from
previous iterations?
-Alan
