How to use pointers correctly in C

Question

I am trying to add (key, value) pairs to a hashmap but cannot access the values after insertion.

This hash table is supposed to deal with collisions as I am iterating along each hash index whenever a collision occurs. I then insert it when I have reached the end of the (key, value) pair list at that index.

Essentially it is a basic linked list hashmap.

The problem is, I keep getting a segmentation fault when I try to access the value again (and my showTable() function also fails). In this test, I am simply trying to access the first (key, value) pair at each hash index after something is added at that hash index. I am probably doing something very silly but I see it.

I have not yet commented but I hope the code is self explanatory. The important bit is InsertKeyValuePair() but I have added everything as a code review would also be beneficial.

#include <stdlib.h>
#include <stdio.h>

typedef struct TVal KeyValue;

typedef struct TVal {
    char *value;
    char *key;
    KeyValue *next;
} KeyValue;

typedef KeyValue **HashTable;

int MAX_SIZE = 200;

int HashKey(char *Key, int Max);
void InsertKeyValuePair(char *key, char *value, int Index, HashTable table);
int insert(char *Key, char *value, HashTable table, int size);
void showTable(HashTable table, int size);

int HashKey(char *Key, int Max) {
    char c = *Key;
    int Hash = 0;
    int n = 1;

    while (c != 0) {
        Hash += n * ((int)c);
        c = *(Key + n);
        n++;
    }

    return Hash % MAX_SIZE;
}

void InsertKeyValuePair(char *key, char *value, int Index, HashTable table) {
    KeyValue *cursor = *(table + Index);
    while (cursor != NULL) {
        cursor = cursor->next;
    }
    cursor = malloc(sizeof(KeyValue));
    cursor->value = value;
    cursor->key = key;
    printf("insert <K,V>(%s,%s) HashIndex = %i\n", cursor->key, cursor->value, Index);

    //Trying to access value previously inserted
    KeyValue *cursor2 = *(table + Index);
    printf("<K,V>(%s,%s)\n", cursor2->key, cursor2->value);
}

int insert(char *Key, char *value, HashTable table, int size) {
    int Index = HashKey(Key, MAX_SIZE);
    InsertKeyValuePair(Key, value, Index, table);
    return size + 1;
}

void showTable(HashTable table, int size) {
    int i;
    for (i = 0; i < size; i++) {
        KeyValue *cursor = *(table + i);

        if (cursor == NULL) 
            continue;

        while (cursor != NULL) {
            printf("==============");
            printf("<K,V>(%s,%s)\n", cursor->key, cursor->value);
            cursor = cursor->next;
        }   
        printf("==============");
    }
}

int main() {
    HashTable HTbl = malloc(sizeof(HashTable) * MAX_SIZE);
    int size = 0;

    size = insert("yeuydfdan", "wesfg", HTbl, size);
    size = insert("ywere", "rdgg", HTbl, size);
    size = insert("ye4", "3244", HTbl, size);

    //showTable(HTbl, MAX_SIZE);
}

Answer 1

There are multiple problems in your code:

• The hash table is not initialized to NULL, causing segmentation faults when trying to dereference the pointers it contains. Allocating with calloc() would fix this problem.
• It is confusing and error prone to hide pointers behind typedefs.
• The allocation in main should read HashTable HTbl = calloc(sizeof(*HTbl), MAX_SIZE);
• the insertion code in InsertKeyValuePair does not link the new pair at the end, nor at the beginning of the hashtable bucket list.
• it is advisable to use unsigned arithmetics to compute the hash key to avoid overflow issues.
• the pointer notation *(table + Index) is confusing. You should use the array notation table[Index] instead.
• there seems to be some confusion between the length of the hashtable (MAX_SIZE) and the number of entries in the hashtable (size). Renaming the variables appropriately may improve readability. It is also probably better to pass the count by address and return a success indicator.

Here is a corrected version:

#include <stdlib.h>
#include <stdio.h>

typedef struct TVal KeyValue;

typedef struct TVal {
    const char *value;
    const char *key;
    KeyValue *next;
} KeyValue;

typedef KeyValue **HashTable;

static unsigned int HASH_SIZE = 200;

static unsigned int HashKey(const char *key);
static KeyValue *InsertKeyValuePair(const char *key, const char *value, int index, HashTable table);
static int insert(const char *Key, const char *value, HashTable table, int *countp);
static void showTable(HashTable table);

static unsigned int HashKey(const char *key) {
    unsigned int hash = 0;
    size_t n;

    for (n = 0; key[n] != 0; n++) {
        hash += n * (unsigned char)key[n];
    }
    return hash % HASH_SIZE;
}

static KeyValue *InsertKeyValuePair(const char *key, const char *value, int index, HashTable table) {
    KeyValue *cursor;
    cursor = malloc(sizeof(KeyValue));
    if (cursor != NULL) {
        KeyValue **cursorp = &table[index];
        while (*cursorp != NULL) {
            cursorp = &(*cursorp)->next;
        }
        cursor->value = value;
        cursor->key = key;
        cursor->next = NULL;
        *cursorp = cursor;
    }
    return cursor;
}

static int insert(const char *key, const char *value, HashTable table, int *countp) {
    int index = HashKey(key);
    if (InsertKeyValuePair(key, value, index, table)) {
        *countp += 1;
        return 1;
    }
    return 0;
}

static void showTable(HashTable table) {
    unsigned int i;
    for (i = 0; i < HASH_SIZE; i++) {
        KeyValue *cursor = table[i];

        if (cursor == NULL)
            continue;

        while (cursor != NULL) {
            printf("==============");
            printf("<K,V>(%s,%s)\n", cursor->key, cursor->value);
            cursor = cursor->next;
        }
        printf("==============\n");
    }
}

int main() {
    HashTable HTbl = calloc(sizeof(*HTbl), HASH_SIZE);
    int count = 0;

    insert("yeuydfdan", "wesfg", HTbl, &count);
    insert("ywere", "rdgg", HTbl, &count);
    insert("ye4", "3244", HTbl, &count);

    showTable(HTbl);
    return 0;
}