On 10/23/2012 8:50 PM, Mike Duigou wrote:
I'm eager to use this!
Some comments:
-<tt>code</tt> is used in some places rather than {@code }
fixed
- The initial<ul> would perhaps be better as a<dl>. I don't know if definition
lists are allowed in javadoc.
- public OutputStream wrap(OutputStream os) closing the underlying output
stream precludes many potential uses. FilterOutputStreams do not consistently
close the underlying stream.
It has been an issue for a while. Obviously there are users that prefer
the underlying stream not
get closed when the outer one is closed. I think we have couple rfes
that ask for a "close this
stream only but not the underlying one" close. But I don't think we can
do this in the default
close(), otherwise, I'm sure we will gen another side of complain, the
close() does not close the
underlying stream:-)
- spelling error "methold"
fixed
- InputStream wrap() it might be nice if the return stream EOF when the padding
character(s) are consumed but not close() or otherwise alter the source
InputStream
I need give it a little more time tomorrow.
- private static byte[] getBytes(ByteBuffer bb) should document, even though
it's a private method, that the bytes returned should be not considered mutable
since they might be shared.
I have been thinking of rewriting the de/encode(ByteBuffer) to get rid
of getBytes(...). Probably
need to move some piece around. If I end up keeping it I will put some
comment int.
- the "if (ret != dst.length) // TBD: not necessary" is worrisome as it will
generate garbage.
while I'm pretty sure I don't need this for encode (it is definitely
needed for
decode()), I just keep it for safe:-) I removed the "TBD" comment though.
- I'm still wishing String encode(byte[] buf) didn't use an intermediate byte
array. :-)
- "stream after use, in which" -> "stream after use, during which"
fixed.
Thanks for taking a look.
-Sherman
On Oct 23 2012, at 19:56 , Xueming Shen wrote:
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
