Review/comment needed for the new public java.util.Base64 class (original) (raw)
Ulf Zibis Ulf.Zibis at CoSoCo.de
Tue Oct 30 16:02:58 UTC 2012
- Previous message: Review/comment needed for the new public java.util.Base64 class
- Next message: Review/comment needed for the new public java.util.Base64 class
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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
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
- Previous message: Review/comment needed for the new public java.util.Base64 class
- Next message: Review/comment needed for the new public java.util.Base64 class
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]