patterncppMinor
HashTable - separate chaining collision in C++
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:
Is it any good? Any advice that will help me improve is welcome!
#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:
Read this
-
From "HASHSET_H_" I gather it's a header file.
You should never use
Link 1
Link 2
-
If you can use C++11 then
// Note, however, that if you're NOT using C++11, then the following is a potential performance problem:
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)
-
In particular, this is very bad:
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
Prefer
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.
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.