Skip to main content
2 of 2
added 281 characters in body
chux
  • 36.4k
  • 2
  • 43
  • 97

Hopefully new points not fully covered by the good 2 prior reviews.


1 Rather than types HashTable..., functions ht_....() and file names hash...., use a common prefix.

2 Instead of allocating to the size of a type, allocate to the size of the referenced pointer. p = malloc(sizeof *p * n). It is easier to code correctly, review and maintain.

// ht->tab = malloc(sizeof(TableEntry_t) * size)
ht->tab = malloc(sizeof *(ht->tab) * size)

3 No need for struct TableEntry definition in hash.h, just declare it and defined this private type in hash.c

// hash.h
typedef struct TableEntry TableEntry_t;

// hash.c
typedef struct TableEntry {
  struct TableEntry *next;
  char *key;
  char *val;
} TableEntry_t;

4 ht_insert() could simply return a bool to indicate success.

5 ht_resize() lacks memory allocation failure test.

6 free(NULL) is OK code. Recommend that ht_free(NULL) also follow that idiom and not seg-fault like it does now. Simple use a if (ht) { existing body }

7 Code is not tolerant of size == 0. If code had a large array of hash tables, it is easy to want those tables of minimal size (0). Then re-size once they get going.

// if ((ht = malloc(sizeof(HashTable_t))) == NULL)
if ((ht = malloc(sizeof(HashTable_t))) == NULL && size > 0)
    return NULL;

8 Weak mixing types and width - best to use size_t for array sizing/indexing.

... size_t size
// int i;
// for (i = 0; i < size; i++)
for (size_t i = 0; i < size; i++)
    ht->tab[i] = NULL;

9 I realize this is C code, yet is is not to hard to avoid C++ keywords like new. As this function is not used outside hash.c, it should be static.

// TableEntry_t *new(char *k, char *v)
static TableEntry_t *ht_new(char *k, char *v)
chux
  • 36.4k
  • 2
  • 43
  • 97