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

[Ulf Zibis]

Am 24.10.2012 04:56, schrieb Xueming Shen:
> 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

Hi Sherman,

my additional comments:

I'm "confused" about the order of xxcode() and Xxcoder.
In other places e.g. charsets, we have de... before en..., which is also true alphabetical

1026     private Base64() {}
1027     private static final int MIMELINEMAX = 76;
1028     private static final byte[] crlf = new byte[] {'\r', '\n'};
- difficult to find, please move (more) to the beginning, at least not in between inner class 
definitions.
- I more would like CRLF than crlf

  118      * @param lineLength    the length of each output line (rounded down
  119      *                      to nearest multiple of 4).
This could be interpreted as: the parameter must be rounded before, so maybe better:
  118      * @param lineLength    the length of each output line (if not a multiple of 4,
  119      *                      it will be rounded down accordingly).


  126     public static Encoder getEncoder(int lineLength, byte[] lineSeparator) {
  127          Objects.requireNonNull(lineSeparator);
  128          return new Encoder(false, lineSeparator, lineLength / 4 * 4);
  129     }
- What (should) happen(s), if lineSeparator.length == 0 ?
- Isn't one of these little faster, or at least more clear? :
     lineLength -= lineLength % 4
     lineLength & 0xFFFFFFFC
     lineLength << 2 >> 2

Broken indentation (why at all, compared to lines 213..215?):
  208         private final byte[] newline;
  209         private final int    linemax;
  210         private final boolean isURL;

Unconventional indentation and even broken in lines 223..224 (maybe more?):
  247          * @param   src   the byte array to encode
  248          * @param   dst   the output byte array
  249          * @return        The number of bytes written to the output byte array
  250          *
  251          * @throws        IllegalArgumentException if {@code dst} does not have
  252          *                enough space for encoding all input bytes.

More conventional:
  247          * @param  src  the byte array to encode
  248          * @param  dst  the output byte array
  249          * @return The number of bytes written to the output byte array
  250          *
  251          * @throws IllegalArgumentException if {@code dst} does not have
  252          *         enough space for encoding all input bytes.

  603         static {
  604             Arrays.fill(fromBase64, -1);
  605             for (int i = 0; i < Encoder.toBase64.length; i++)
  606                 fromBase64[Encoder.toBase64[i]] = i;
  607             fromBase64['='] = -2;
  608         }
This causes the inner Encoder class to be loaded, even if not needed. So maybe move the toBase64 
table to the outer class.
Have you compared performance with fromBase64 as byte[] (including local 'b' in decode() as byte)?
Retrieving the bytes by b = base64[x] then could be done without address-shift and smaller/faster 
LoadB operations could be used by JIT. At least the footprint of the table would be smaller.

Missing spaces:
  805             return new DecInputStream(is, isURL?fromBase64URL:fromBase64, isMIME);
Why at all you repeat this code many times? Why not? :
  631             this.base64 = isURL ? fromBase64URL : fromBase64;
Also it would at least be helpful for readers to define final, e.g.:
  809             final int[] base64 = isURL ? fromBase64URL : fromBase64;

Why you mix the algorithms to check the padding? :
  824                         if (b == -2) {   // padding byte
  890                     if (b == '=') {


-Ulf