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

[Xueming Shen]

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