Review/comment needed for the new public java.util.Base64 class

[Alan Bateman]

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