ctubbsii commented on code in PR #80:
URL: https://github.com/apache/accumulo-access/pull/80#discussion_r1692507716
##########
src/main/java/org/apache/accumulo/access/BytesWrapper.java:
##########
@@ -54,15 +55,7 @@ public BytesWrapper(byte[] data) {
}
byte byteAt(int i) {
-
- if (i < 0) {
- throw new IllegalArgumentException("i < 0, " + i);
- }
-
- if (i >= length) {
- throw new IllegalArgumentException("i >= length, " + i + " >= " +
length);
- }
-
+ Objects.checkIndex(i, length);
Review Comment:
You can make this more succinct by doing
```suggestion
return data[offset + Objects.checkIndex(i, length)];
```
I would probably also statically import `checkIndex` to make it even more
concise.
##########
src/main/java/org/apache/accumulo/access/BytesWrapper.java:
##########
@@ -21,12 +21,13 @@
import static java.nio.charset.StandardCharsets.UTF_8;
import java.util.Arrays;
+import java.util.Objects;
final class BytesWrapper implements Comparable<BytesWrapper> {
- protected byte[] data;
- protected int offset;
- protected int length;
+ private byte[] data;
+ private int offset;
+ private int length;
Review Comment:
I wonder if it's worth creating an internal tuple object so that these can
be set atomically to make this a bit more thread safe. You never know how users
are going to use this library, and they might call `set` in one thread while
calling `byteAt` in a different thread. If this were a tuple updated
atomically, then you could guarantee a consistent view in any single method
that tried to read the internal backing array, without having to worry about a
partial update that swapped out the data, but hasn't yet updated the offset and
length.
##########
src/main/java/org/apache/accumulo/access/BytesWrapper.java:
##########
@@ -118,18 +111,7 @@ public String toString() {
* a copy of the input buffer
*/
void set(byte[] data, int offset, int length) {
- if (offset < 0) {
- throw new IllegalArgumentException("Offset cannot be negative. length =
" + offset);
- }
- if (length < 0) {
- throw new IllegalArgumentException("Length cannot be negative. length =
" + length);
- }
- if ((offset + length) > data.length) {
- throw new IllegalArgumentException(
- "Sum of offset and length exceeds data length. data.length = " +
data.length
- + ", offset = " + offset + ", length = " + length);
- }
-
+ Objects.checkFromIndexSize(offset, length, data.length);
Review Comment:
I like that this method is provided built-in. I don't like that it takes 3
ints, and 2 of them have the same semantics of "size of byte string". I had to
quadruple check the javadoc and the arguments to make sure the method was being
called correctly. I hate that it'd be really easy to mess this up. That said,
this looks correct to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
