Custom Hashset in Java

Question

I have written this custom hashset and though it isn't completed yet, I would like to know if there is anything I am overlooking in terms of clean code conventions. My aim was also to apply generics to it. So, would also want any input regarding that.

 public class CustomHashset<T> {
 private static final int SIZE = 100;
 private Entry<T>[] buckets;
 private int size;
 public CustomHashset() {
 this.buckets = new Entry[SIZE];
 this.size= 0;
 }
 private int hash(T element) {
 return element.hashCode() % buckets.length;
 }
 public Boolean add(T element) {
 int index = hash(element);
 Entry<T> current = buckets[index];
 while (current!=null) {
 if (current.key.equals(element)) return false;
 current = current.next;
 }
 Entry<T> entry = new Entry<T>();
 entry.setKey(element);
 entry.setNext(buckets[index]); 
 buckets[index] = entry;
 size++;
 return true;
 }
 public int size() {
 return size;
 }
 private static class Entry<T> {
 private T key;
 private Entry next;
 public T getKey() {
 return key;
 }
 public void setKey(T element) {
 this.key = element;
 }
 public Entry getNext () {
 return next;
 }
 public void setNext(Entry next) {
 this.next = next;
 }
 }
}

• DapperDan: Request for Clarification: do you want your CustomHashSet to be a HashSet? i.e., do you want this to be valid: HashSet<T> hashSet = new CustomHashSet<>();
• user200188: @Wood Glass, no.
• Benjamin Kuykendall: What are your design goals? Is there a reason that java.util.HashSet will not suffice? Do you want to integrate nicely with the Java Collections Framework, or avoid it entirely? It is hard to review this code without more context.
• user200188: @BenjaminKuykendall, simply learning how to implement these data structures using generics.

Answer 1

Problems with generics

• Entry<T>.next should have type Entry<T>.

• CustomHashset<T>.buckets has type Entry<T>[] but is initialized to an Entry[]. Initializing a generic array properly is annoying, but this StackOverflow question explores a few solutions. At the end of the day, there's only so much you can do about it; you can at least suppress the compiler warning though.

Interface of Entry

You never use the getters and setters. Remove them.

Abstract out linked list

As you implement more methods, it should become apparent that you are using the chain of Entry<T> objects as a linked list. Thus the interface it presents could be more abstract: in fact, I would create a LinkedList<T> class with add(T t) and contains(T t) methods. Then make buckets an array of LinkedList<T>. This way, the hash set never has to deal with individual entries.

Small problems

• Use or omit this more consistently. I would omit it unless needed.
• Return boolean not Boolean.
• Currently, a null key causes a NullPointerException when you call hash(). You should detect null inputs to add and explicitly throw an IllegalArgumentException instead.
• Use default visibility instead of public to expose methods of private inner classes.

• user200188: Thanks for your review. Very interesting take. 1. I really didn't have any problem initializing the generic array. Maybe there is something you can point to in the way I have initialized it for a better understanding. I took a look at the link you added and it appears I was doing something close to the checked except I wasn't wrapping it in the way it was explained over there. 2. I did use the setters but not the getters. So, I will remove the getters. Thanks for that. Continuation ...below
• user200188: 3. Could you elaborate further on the 'Abstract out linked list'? I get you want me to make the Entry<T> a LinkedList<T> because, under the hood, I was using node chaining. I don't get making buckets an array of LinkedList<T>. Even though I could do that, would I not be relying on an existing implementation? I could very well have just added a Java HashMap internally and not bother about implementing the hashing and the add contains e.t.c as I would just be calling the hashMap operators under the hood. So, just want to know if this is Ok to do.
• Benjamin Kuykendall: I would also ditch the setters in favor of a constructor: Entry<T>(T element, Entry<T> next). If you do this, you can even make both fields final. If you don't want to use java.util.LinkedList, writing your own CustomLinkedList that supports add and contains is really easy; in fact you have most of the code there, it's just a mater of reorganizing it.

Answer 2

Improving names:

• Your class has 2 size variables, which looks kinda strange. Better to rename this private static one to something like INITIAL_CAPACITY (or DEFAULT_INITIAL_CAPACITY, if you have plans adding constructor with initialCapacity parameter), because size = how many elements are stored, but this variable denotes initial array length.

• method hash in reality returns bucket index for given element, thus, should be renamed. For example, to indexFor or bucketIndex.

Other:

• in Entry - next is not parameterized (as well as its getter and setter).