1

I'm learning hash tables and found this code on another website, but am having trouble understanding the Insert(int key, int value) function. The function works well but I am wondering if there is extra code that is not needed or if I am not understanding it completely.

Specifically, the else condition at the end of the function:

else
{
     entry->value = value;
 }

It doesn't seem to ever reach that condition when I call that function using different parameters. Here is the rest of the code.

#include<iostream>
#include<cstdlib>
#include<string>
#include<cstdio>
using namespace std;
const int TABLE_SIZE = 128;

class HashNode
{
public:
    int key;
    int value;
    HashNode* next;
    HashNode(int key, int value)
    {
        this->key = key;
        this->value = value;
        this->next = NULL;
    }
};

class HashMap
{
private:
    HashNode** htable;
    public:
    HashMap()
    {
        htable = new HashNode*[TABLE_SIZE];
        for (int i = 0; i < TABLE_SIZE; i++)
            htable[i] = NULL;
    }
    ~HashMap()
    {
        for (int i = 0; i < TABLE_SIZE; ++i)
        {
            HashNode* entry = htable[i];
            while (entry != NULL)
            {
                HashNode* prev = entry;
                entry = entry->next;
                delete prev;
            }
        }
        delete[] htable;
    }
    /*
     * Hash Function
     */
    int HashFunc(int key)
    {
        return key % TABLE_SIZE;
    }

    /*
     * Insert Element at a key
     */
    void Insert(int key, int value)
    {
        int hash_val = HashFunc(key);
        HashNode* prev = NULL;
        HashNode* entry = htable[hash_val];
        while (entry != NULL)
        {
            prev = entry;
            entry = entry->next;
        }
        if (entry == NULL)
        {
            entry = new HashNode(key, value);
            if (prev == NULL)
            {
                htable[hash_val] = entry;
            }
            else
            {
                prev->next = entry;
            }
        }
        else
        {
            entry->value = value;
        }
    }

    /* Search Element at a key
     */
    int Search(int key)
    {
        bool flag = false;
        int hash_val = HashFunc(key);
        HashNode* entry = htable[hash_val];
        while (entry != NULL)
        {
            if (entry->key == key)
            {
                cout << entry->value << " ";
                flag = true;
            }
            entry = entry->next;
        }
        if (!flag)
            return -1;
    }
};

int main()
{
    HashMap hash;
    hash.Insert(3, 7);
    hash.Search(3);
}

Any clarification is highly appreciated.

Thank you

ldc57
  • 23
  • 6
  • First thing to do is sort out the indentation. Bad indentation makes code much, much harder to interpret. – user4581301 Mar 04 '17 at 22:47
  • Now that is out of the way, let's take a look... – user4581301 Mar 04 '17 at 22:48
  • It;s just a redundant bit of code that got its way in. As you observe, entry is guaranteed to be null because of the loop. If both hash and key match, you do indeed set value, but that is dealt with elsewhere. – Malcolm McLean Mar 04 '17 at 23:31

1 Answers1

2
while (entry != NULL)

precedes

if (entry == NULL) 

There is no way out of the while (entry != NULL) loop unless entry is NULL, guaranteeing that the else case is impossible.

I believe that inside the while loop a test to see if the key is already present is required.

while (entry != NULL)
{
    if (entry->key == key)
    {
        break;
    }
    prev = entry;
    entry = entry->next;
}

Off topic: Take a look at this question and answer for a suggestion on how to simplify your code: Using pointers to remove item from singly-linked list

Example:

/*
 * Insert Element at a key
 */
void Insert(int key, int value)
{
    int hash_val = HashFunc(key);
    HashNode** nextPtr = &htable[hash_val]; // Get a pointer to next, not to the entry
    // with a pointer to next we can keep going from next to next without 
    // ever needing a previous.
    // pick up a whole bunch of extra dereferencing *nextPtr, but an 
    // optimizing compiler is great at dealing with that extra overhead.
    while ((*nextPtr) != NULL) // loop until found or end
    {
        if ((*nextPtr)->key == key) // found key already in list
        {
            (*nextPtr)->value = value; // set value for key
            return; // all done.
        }
        nextPtr = &(*nextPtr)->next; // keep looking
    }
    *nextPtr = new HashNode(key, value); // didn't find. Add new node to end.
}

Addendum: Search only returns in the failure case. A function that is declared as returning a value must always return a value.

Community
  • 1
  • 1
user4581301
  • 33,082
  • 7
  • 33
  • 54
  • It seems like they meant to break out of the `while` loop if the key is already in the hash table, but forgot to do so. – interjay Mar 04 '17 at 22:54
  • @interjay Most likely possibility. If the entry is in the hash table though, I'm not sure what their plan is. Chain or refuse to insert? Will Tweak answer anyway. Not much of an answer without at least an attempt at a solution, is it? – user4581301 Mar 04 '17 at 22:57
  • 1
    The `else` changes the value of an existing entry, so I suppose that is the intention. All it would take is adding `&& entry->key != key` to the `while` condition. – interjay Mar 04 '17 at 23:00
  • Thank you very much . Super fast responses . I also had a question about the Search function . I dont see how the entry->value is outputting multiple values if there is a "collision with multiple values" . It works and outputs multiple values with the same key . But I only see one pointer which is entry that could dereference to entry->value . Dont see how it holds separate multiple values at entry->value – ldc57 Mar 05 '17 at 00:05
  • @ldc57 as far as I can see it only prints one value, if one exists. The problem is if a value exists it does not return an `int` as was specified by `int Search(int key)` resulting in an invalid program that can do amazingly weird things. On finding a match, I would `return entry->value;` and make the caller print the found value. – user4581301 Mar 05 '17 at 00:41
  • Thx for the link user4581301 . I will check it out . – ldc57 Mar 05 '17 at 01:55
  • Yes sorry about the indentation , my first post and had to step out running errands right when sending and ws having a little difficulty with the code box vs the commentary play by play. – ldc57 Mar 05 '17 at 02:09
  • Seems like one of my comments was cut off so here we go again , I inputted hash.Insert(3,7); hash.Insert(3,8); hash.Insert(3,9); and it outputted 7 8 9 so it seems to outputs multiple values via cout and not return int . And thats what is confusing me that the program seems to be invalid in a few ways . I will look at other sources on how to implement a hash table in c++. If any recommendations please let me know . Thank you – ldc57 Mar 05 '17 at 02:14
  • I've made two changes to your code, the change to the while loop and a quick fix to `Search` so that it returns 0 at the end of the function no matter what and I am unable to reproduce: http://ideone.com/Owvj6J Best suggestion I have at this point is to ask a new question with your current code and your new problem. – user4581301 Mar 05 '17 at 03:12
  • Reading the suggestions again , yes either : if (entry->key == key) { break; } or interjay's suggestion of && entry->key !=key breaks out of the while loop and gives only one value , not multiple values – ldc57 Mar 05 '17 at 03:34
  • And user458 , yes , sorry I meant with the original code , it was outputting multiple values of 7 8 9 . Not with your modifications . With your modifications , yes there is only one value even when adding same key and multiple different values . Thank you I will give thumbs up to you shortly when I get to it . – ldc57 Mar 05 '17 at 03:41
  • I must say , as mentioned previously , the original code outputs multiple value , like an unordered_multimap . Adding the two pieces of suggested code makes the hash table output single values only for each key , regardless of multiple values being input for the same key . Also , the /* else { entry->value = value ;} */ definitely seems like its extra code that is not necessary . – ldc57 Mar 05 '17 at 07:03
  • Also can see how the linking is working , with **prev** pointing to **entry** in the **Insert** function and **entry = entry->next** in the **Search** function , it's able to while loop through the **new** **entry** addresses and output its values at each address until it's NULL – ldc57 Mar 05 '17 at 20:57
  • @ldc57 You do need a `entry->value = value ;` somewhere to set the value. I just think you don't need the else because it can be better placed. Added an example. – user4581301 Mar 05 '17 at 22:24