Simple Chained HashMap

I am very inexperienced at C++. I wrote a Hash Map which uses chaining for collision resolution. I intentionally did not use smart-pointers, as the objective of this was to learn and showcase. I have tested the code and removed all bugs I came across. I am hoping to get feedback on bad practices and potential risks associated with my code.

HashMap.h

#pragma once
#include 

#include "HashElement.h"

class HashMap
{
private:
    HashElement **map_;
    int size_;
    int count_;
public:
    HashMap(int);
    ~HashMap();
    int GetHash(int);
    void Put(int, std::string);
    std::string GetElement(int);
    bool Contains(int);
    void Remove(int);
    int GetCount();
};

HashMap.cpp

```
#include "HashMap.h"
#include "PrimeChecker.h"

HashMap::HashMap(int size)
{
while (!PrimeChecker::IsPrime(size)){
size++;
}
size_ = size;
map_ = new HashElement*[size_]();
}

HashMap::~HashMap()
{
for (int i = 0; i next_element_){
next_element = next_element->next_element_;
delete current_element;
current_element = next_element;
}
delete current_element;
}
}

int HashMap::GetHash(int key){
return key % size_;
}

void HashMap::Put(int key, std::string value){
int hash = GetHash(key);
if (!map_[hash]){
map_[hash] = new HashElement(key, value);
}
else{
HashElement *last_element = map_[hash];
while (last_element->next_element_){
last_element = last_element->next_element_;
}
last_element->next_element_ = new HashElement(key, value);
}
count_++;
}

std::string HashMap::GetElement(int key){
int hash = GetHash(key);
if (map_[hash]){
HashElement *current_element = map_[hash];
while (current_element->GetKey() != key && current_element->next_element_){
current_element = current_element->next_element_;
}
return current_element->GetValue();
}
return nullptr;
}

I think it is quite good. Without touching on algorithmic and design aspects, I can see a few points that you can further improve with the current code:

-
You are missing Const Correctness on const methods. This is an important aspect of C++ that is not obvious, so it is common for beginners or people migrating from other languages to miss it.

-
This is a bit of a personal suggestion, but I consider the public parts of a class to be more relevant for people reading my code, so I tend to place public methods and data first, followed by protected and finally private members, which are implementation details that only concern the programmer implementing the class(es).

-
I suggest keeping function parameter names in your function prototypes, as this will add to the documentation of the code.

-
For HashMap, I would change a few method names:

Instead of Put to add a value to the map, I would call it Insert, to closer resemble the Standard map data structure that most C++ programmers are familiar with.

Also, instead of GetElement to fetch a value, I would name it Find, again to resemble the Standard map type but also because Get* implies a light weight operation while GetElement might be a linear search in the bucket chain if the value is not the first one.

-
You need to give some thought to the Rule of Three, which is about how your class is going to behave regarding value copy/assignment. As it stands, one could create copies of a HashMap using operator = and the default copy constructor. That would be disastrous, since each instance of a map own its memory and a copy with = would be a shallow copy of just the pointer, leading to duplicate attempts to free the same memory.

The simplest approach would be disabling copy and assignment by making operator = and the copy constructor private (or deleting them). A more robust solution is implementing custom ones that perform a deep copy of the map.

-
HashElement doesn't seem to be used anywhere outside HashMap, so it could be an inner class declared inside the private section of HashMap.

-
The destructor of HashElement is empty, so you should not declare one and let the compiler supply the default.

-
Try to always initialize data in constructors using the constructor initialization list. Taking HashElement as example, the proper way would be:

HashElement::HashElement(int key, std::string value)
      : key_(key)
      , value_(std::move(value))
      , next_element_(nullptr)
  { }

Doing so will ensure that the members are also initialized by the constructor instead of the assignment operator.

Also note std::move() when initializing value_. This is part of the C++11 move semantics, which will remove a redundant copy that you don't need.

-
Consider providing operator [] to allow usage such as:

myMap[42] = "hello world";

The Standard C++ map provides such operator. You might want to use it as a guideline:

class HashMap {
    ...
    std::string& operator[] (int key);
};

std::string& HashMap::operator[] (int key)
{
    // How you'll implement this depends.
    // If you follow the convention of std::map,
    // This method should create a new entry if the
    //  pair is not present, returning a 
    // reference to it one completed.
    // If the entry is already available, 
    // just return a reference to the value.
    //
    // Pseudocode:
    //  if (key !in map)
    //      insert(key, new string);
    //  return ref find(key); 
}

-
The namespace PrimeChecker seems a bit unnecessary and verbose. IsPrime() at the global namespace would be fine too.

StackExchange Code Review Q#83009, answer score: 4