Hi Alan,
Thanks for the review. I hope I addressed most of the comments in the
updated webrev at
http://cr.openjdk.java.net/~sherman/4235519/webrev
mainly
(1) Pulled the base64 "terms" up to the class doc and then be referenced
from various methods
(2) Gave up the C style de/encode(byte[], null), just leave the invoker
to have enough space. I
remember Paul once suggested to have a convenient method to return
the estimated length
of the byte[] needed to en/decode a specified input byte
array/buffer. I think we can do it
later when it is really desired. I agreed your argument that if
people need to get the size and
prepare a "new" array, they bet off just call de/encode(byte[])
(3) Gave up the "liberal" decoding design, tight the spec/impl to treat
the input as "illegal base64"
if the padding character appears in the middle of input.
(4) Some clarification of spec for thread safe, null exception and the
pos/limit of input/output byte
buffer as suggested.
(5) Fixed the typos
I still don't have a better name for method "getUrlEn/Decoder()",
Base64.getBase64UrlEn/Decoder()
does not feel good for me.
-Sherman
On 10/23/12 6:04 AM, Alan Bateman wrote:
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
