HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

HashTable - separate chaining collision in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
chainingcollisionhashtableseparate

Problem

I'm trying to implement a HashTable. In fact, I want a HashSet with Persons that have a name and a phoneNumber, both string. I understood the theory with collisions and everything but I don't know how to really implement something. This is what I've done so far:

#ifndef HASHSET_H_
#define HASHSET_H_

#include 
using namespace std;
#include 

template 
class HashSet{
private:
    class Node{
    private:
        Element info;
        Node* next;
    public:
        Node(){ //constructor
            this->next=NULL;
        }
        Node(Node* next, Element info){ //parameters constructor
            this->info=info;
            this->next=next;
        }
        Node(const Node& node){ //copy constructor
            this->info=node.info;
            this->next=node.next;
        }
        ~Node(){} //destructor
        Element getInfo(){
            return this->info;
        }
        Node* getNext(){
            return this->next;
        }
        void setInfo(Element newE){
            this->info=newE;
        }
        void setNext(Node* value){
            this->next=value;
        }
    };
    Node** head;
    int hashSize;
    int bucketSize; //holds the total elements of a cell
    int totElems; //total number of elements in the hashset
public:
    HashSet(){
    }
    bool IsEmptyAtGivenKey(int key);
    bool HashSetIsFull();
    int HashFunction(string e);
    int HashSetSize();
};

template
HashSet::HashSet(){
    this->hashSize=11;
    this->head=new Node*[hashSize];
    for(int i=0; ihead[i]=NULL;
    }
    this->totElems=0;
}

template
bool HashSet::IsEmptyAtGivenKey(int key){
    if(key>=0 and key 
int HashSet::HashFunction(string e){
    int length = e.size();
    int sum;
    for(int i=0; i < length; i++){
        sum+=e[i];

    }
    int hashCode = sum % hashSize;
    return hashCode;
}

#endif /* HASHSET_H_ */


Is it any good? Any advice that will help me improve is welcome!

Solution

-
You're attempting to read values from uninitialized variables which has undefined behavior:

int sum; // uninitialized`  
for(int i=0; i < length; i++){`  
    sum+=e[i]; // reading from uninitialized variable "sum"


Read this

-
From "HASHSET_H_" I gather it's a header file.
You should never use using namespace std; in a header file (and chances are it's a bad idea to use it anywhere else, too -- unless you're porting legacy 1980s C code to C++):

Link 1

Link 2

-
If you can use C++11 then nullptr is a better choice than NULL: read this

// Note, however, that if you're NOT using C++11, then the following is a potential performance problem:
int HashSet::HashFunction(string e)
Pre-C++11 this can create unnecessary copy, so prefer to pass-by-const-reference in C++98/03 instead.

Compare:

copy

move

no copy (elided)

-
int is a wrong type for a variable holding size or count (such as hashSize, bucketSize, totElems).
In particular, this is very bad: int length = e.size();
Not only you're using a wrong type, but potentially also losing performance due to unsigned-to-signed conversion // which, perhaps more importantly, is completely unnecessary and only makes the code harder to read -- are you suggesting that length can be negative? If not, then don't use int.

Prefer std::size_t instead (defined in header `):

Link 1

Link 2

-
Make sure to use meaningful variable names. This includes not using ambiguous abbreviations.
totElems is not a good name. totalElementsCount is better.

-
Additional idea: make sure to pick a naming convention and stick to it.
For instance, some of your functions start with uppercase character, like
IsEmptyAtGivenKey, but some with a lowercase character, like setInfo.
It's better if they all follow the same convention (e.g., all functions start with lowercase, all types start with uppercase, etc.).

As a hint, read the "Naming" section from Google C++ Style Guide.

// Extra hint: feel free to completely ignore the other sections, or even take them as examples of what NOT to do (like the one on exceptions), unless you're working at Google with Google's code base.

-
Manual memory management is troubling.

I've read from your comments that this is for school, so chances are the class / teaching methods simply aren't exactly up-to-date (thinking teaching C before C++ is a good idea is often a good indicator), but IRL invoking raw
new (such as here: new Node*[hashSize];`) without a very good reason is nowadays unnecessary and considered a bad practice.

If you want to learn best practices, start with this.

// Not to repeat myself from my previous answer, here's a link to it, if you'd like more information on memory management specifically: here.

Code Snippets

int sum; // uninitialized`  
for(int i=0; i < length; i++){`  
    sum+=e[i]; // reading from uninitialized variable "sum"

Context

StackExchange Code Review Q#25867, answer score: 4

Revisions (0)

No revisions yet.