[GitHub] ctubbsii commented on a change in pull request #950: Move TableId and NamespaceId to public API

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #950: Move TableId and NamespaceId to public API
URL: https://github.com/apache/accumulo/pull/950#discussion_r255572064
 
 

 ##########
 File path: core/src/main/java/org/apache/accumulo/core/data/AbstractId.java
 ##########
 @@ -14,78 +14,56 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.apache.accumulo.core.clientImpl;
-
-import static java.nio.charset.StandardCharsets.UTF_8;
-import static java.util.Objects.requireNonNull;
+package org.apache.accumulo.core.data;
 
 import java.io.Serializable;
 import java.util.Objects;
 
 /**
  * An abstract identifier class for comparing equality of identifiers of the same type.
  */
-public abstract class AbstractId implements Comparable<AbstractId>, Serializable {
+public abstract class AbstractId<T extends AbstractId<T>> implements Comparable<T>, Serializable {
 
-  private static final long serialVersionUID = -155513612834787244L;
+  private static final long serialVersionUID = 1L;
   private final String canonical;
-  private Integer hashCode = null;
 
   protected AbstractId(final String canonical) {
-    requireNonNull(canonical, "canonical cannot be null");
-    this.canonical = canonical;
+    this.canonical = Objects.requireNonNull(canonical, "canonical cannot be null");
   }
 
   /**
-   * The canonical ID
+   * The canonical ID. This is guaranteed to be non-null.
    */
-  public final String canonicalID() {
+  public final String canonical() {
     return canonical;
   }
 
-  public boolean isEmpty() {
-    return canonical.isEmpty();
-  }
-
   /**
    * AbstractID objects are considered equal if, and only if, they are of the same type and have the
    * same canonical identifier.
    */
   @Override
   public boolean equals(final Object obj) {
-    if (this == obj)
-      return true;
-    return obj != null && Objects.equals(getClass(), obj.getClass())
-        && Objects.equals(canonicalID(), ((AbstractId) obj).canonicalID());
+    return this == obj || (obj != null && Objects.equals(getClass(), obj.getClass())
+        && Objects.equals(canonical(), ((AbstractId<?>) obj).canonical()));
   }
 
   @Override
   public int hashCode() {
-    if (hashCode == null) {
-      hashCode = Objects.hash(canonicalID());
-    }
-    return hashCode;
+    return canonical().hashCode();
   }
 
   /**
    * Returns a string of the canonical ID
    */
   @Override
   public String toString() {
-    return canonical;
-  }
-
-  /**
-   * Return a UTF_8 byte[] of the canonical ID.
-   */
-  public final byte[] getUtf8() {
-    return canonical.getBytes(UTF_8);
+    return canonical();
   }
 
   @Override
-  public int compareTo(AbstractId id) {
-    requireNonNull(id, "id cannot be null");
-    return this.canonicalID().compareTo(id.canonicalID());
+  public int compareTo(T other) {
 
 Review comment:
   Not in any normal situation. equals checks type and canonical field. compareTo compares whatever the generic parameter type is. So, in order for that to happen, one would have to do something like `public class NewIdType extends AbstractId<TableId>` or subclass TableId, and not override equals.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services