You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@fluo.apache.org by cjmctague <gi...@git.apache.org> on 2017/04/22 01:41:29 UTC
[GitHub] incubator-fluo pull request #825: fixes #823
GitHub user cjmctague opened a pull request:
https://github.com/apache/incubator-fluo/pull/825
fixes #823
-added startsWith(Byte prefix) method to Bytes.java
-added endsWith(Bytes suffix) method to Bytes.java
-added testPrefixSuffix() method to BytesTest.java
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cjmctague/incubator-fluo fluo-823
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-fluo/pull/825.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #825
----
commit d47427cc343bd75b2c1d6c637d5342e7b526ec5f
Author: Christopher McTague <cj...@vwc.edu>
Date: 2017-04-22T00:25:03Z
fixes #823
-added startsWith(Byte prefix) method to Bytes.java
-added endsWith(Bytes suffix) method to Bytes.java
-added testPrefixSuffix() method to BytesTest.java
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112800350
--- Diff: modules/api/src/test/java/org/apache/fluo/api/data/BytesTest.java ---
@@ -79,6 +79,19 @@ public void testHashSubsequence() {
}
@Test
+ public void testPrefixSuffix() {
+ Bytes b1 = Bytes.of("abcde");
+ Bytes prefix = Bytes.of("ab");
+ Bytes suffix = Bytes.of("de");
+ Bytes mid = Bytes.of("cd");
+
+ Assert.assertEquals(true, b1.startsWith(prefix));
+ Assert.assertEquals(true, b1.endsWith(suffix));
+ Assert.assertEquals(false, b1.startsWith(mid));
+ Assert.assertEquals(false, b1.endsWith(mid));
--- End diff --
Should use `Assert.assertTrue` and `Assert.assertFalse` instead of `Assert.assertEquals` here.
Also, a few more cases would be helpful for full coverage (these, plus yours, I *think* cover all cases):
```
true =? "".startsWith("") // trivial empty check
true =? "".endsWith("") // trivial empty check
true =? "abc".startsWith("abc") // exact match
true =? "abc".endsWith("abc") // exact match
true =? "abc".startsWith("") // empty string exists at start
true =? "abc".endsWith("") // empty string exists at end
false =? "".startsWith("abc") // empty string can't start with longer string
false =? "".endsWith("abc") // empty string can't end with longer string
false =? "a".startsWith("abc") // regular string can't start with longer string
false =? "a".endsWith("abc") // regular string can't end with longer string
```
Could also check `Bytes.EMPTY` with `Bytes.of(String)` or `Bytes.of(CharSequence)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo issue #825: fixes #823
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on the issue:
https://github.com/apache/incubator-fluo/pull/825
Thanks @cjmctague ! I squashed and merged the change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112800518
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -355,6 +354,36 @@ public static final Bytes of(String s, Charset c) {
}
/**
+ * Checks if this has the passed prefix
+ *
+ * @param prefix
+ * @return true or false
+ */
+ public boolean startsWith(Bytes prefix) {
+ for (int i = 0; i < prefix.length; i++) {
--- End diff --
If prefix length is longer than this length, this should return false.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112800313
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -355,6 +354,36 @@ public static final Bytes of(String s, Charset c) {
}
/**
+ * Checks if this has the passed prefix
+ *
+ * @param prefix
+ * @return true or false
+ */
+ public boolean startsWith(Bytes prefix) {
+ for (int i = 0; i < prefix.length; i++) {
+ if (this.byteAt(i) != prefix.byteAt(i))
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * Checks if this has the passed suffix
+ *
+ * @param suffix
+ * @return true or false
+ */
+ public boolean endsWith(Bytes suffix) {
+ int startOffset = this.length - suffix.length;
+ for (int i = startOffset; i < this.length; i++) {
--- End diff --
if startOffset is negative here, this should return false. In other words, it is always impossible for a string to be a suffix of a shorter string.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r113102707
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -356,29 +357,44 @@ public static final Bytes of(String s, Charset c) {
/**
* Checks if this has the passed prefix
*
- * @param prefix
+ * @param prefix is a Bytes object to compare to this
* @return true or false
+ * @since 1.1.0
*/
public boolean startsWith(Bytes prefix) {
- for (int i = 0; i < prefix.length; i++) {
- if (this.byteAt(i) != prefix.byteAt(i))
- return false;
+ Objects.requireNonNull(prefix, "startWith(Bytes prefix) cannot have null parameter");
+
+ if (prefix.length > this.length) {
+ return false;
+ } else {
+ for (int i = 0; i < prefix.length; i++) {
--- End diff --
To avoid repeatedly adding offsets, could do something like the following.
```java
int end = this.offset + prefix.length;
for (int i = this.offset, j = prefix.offset; i < end; i++,j++)
```
I am not completely sure about the syntax.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112968192
--- Diff: modules/api/src/test/java/org/apache/fluo/api/data/BytesTest.java ---
@@ -79,6 +79,19 @@ public void testHashSubsequence() {
}
@Test
+ public void testPrefixSuffix() {
+ Bytes b1 = Bytes.of("abcde");
+ Bytes prefix = Bytes.of("ab");
+ Bytes suffix = Bytes.of("de");
+ Bytes mid = Bytes.of("cd");
+
+ Assert.assertEquals(true, b1.startsWith(prefix));
+ Assert.assertEquals(true, b1.endsWith(suffix));
+ Assert.assertEquals(false, b1.startsWith(mid));
+ Assert.assertEquals(false, b1.endsWith(mid));
--- End diff --
Some test cases that compare with self would be nice. I have had problems in the past with implementations that do not properly handle this case. Also, test with subsequence (which will create a new Bytes that has the same inner array) would be nice.
```java
assertTrue(b1.startsWith(b1));
assertTrue(b1.endsWith(b1));
assertTrue(b1.startsWith(b1.subSequence(0,2));
assertFalse(b1.subSequence(0,2).startsWith(b1));
//do similar test with subseq and endswith
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112800248
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -355,6 +354,36 @@ public static final Bytes of(String s, Charset c) {
}
/**
+ * Checks if this has the passed prefix
+ *
+ * @param prefix
+ * @return true or false
+ */
+ public boolean startsWith(Bytes prefix) {
+ for (int i = 0; i < prefix.length; i++) {
+ if (this.byteAt(i) != prefix.byteAt(i))
--- End diff --
`mvn clean verify` fails with checkstyle. Fluo prefers using braces, even for one-line if blocks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112800262
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -355,6 +354,36 @@ public static final Bytes of(String s, Charset c) {
}
/**
+ * Checks if this has the passed prefix
+ *
+ * @param prefix
+ * @return true or false
--- End diff --
Would be useful to add a `@since 1.1.0` javadoc tag to document when the new API method first appeared.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-fluo/pull/825
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by keith-turner <gi...@git.apache.org>.
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112973359
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -355,6 +354,36 @@ public static final Bytes of(String s, Charset c) {
}
/**
+ * Checks if this has the passed prefix
+ *
+ * @param prefix
+ * @return true or false
+ */
+ public boolean startsWith(Bytes prefix) {
+ for (int i = 0; i < prefix.length; i++) {
+ if (this.byteAt(i) != prefix.byteAt(i))
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * Checks if this has the passed suffix
+ *
+ * @param suffix
+ * @return true or false
+ */
+ public boolean endsWith(Bytes suffix) {
+ int startOffset = this.length - suffix.length;
+ for (int i = startOffset; i < this.length; i++) {
+
+ if (this.byteAt(i) != suffix.byteAt(i - startOffset))
--- End diff --
Looking at this code prompted me to open #826. Would be ok to make these improvements for endsWith and startsWith in this PR or in a later PR for #826 that improves all the code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-fluo pull request #825: fixes #823
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/incubator-fluo/pull/825#discussion_r112800377
--- Diff: modules/api/src/main/java/org/apache/fluo/api/data/Bytes.java ---
@@ -355,6 +354,36 @@ public static final Bytes of(String s, Charset c) {
}
/**
+ * Checks if this has the passed prefix
+ *
+ * @param prefix
+ * @return true or false
+ */
+ public boolean startsWith(Bytes prefix) {
+ for (int i = 0; i < prefix.length; i++) {
--- End diff --
prefix (and suffix in next method) should be checked for null with `Objects.requiresNonNull(prefix)`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---