The
hashtable[i] = malloc(sizeof(node));
hashtable[i]-> next = NULL;
makes no sense to me. Why create a node if you don't have content for it yet?
The hashtable[i] = NULL;
also doesn't add much, as global variables are guaranteed to start initialised to zero (unlike function-scope variables).
Within the loop, you then replace the word stored in one of those 26 nodes, but you have at most 26 nodes with valid words (the rest of them might contain arbitrary data, as malloc
doesn't specify the content of the allocated memory).
You should however malloc
(or equivalent) for each new word, within the while (fscanf...
loop. Something like
node *newnode = (node*)malloc(sizeof(node));
newnode->next = hashtable[key];
hashtable[key] = newnode;
would install a new node at the beginning of linked list hashtable[key]
.
For the "clean up", notice that char
is a (signed or unsigned, with the default being implementation-specific) 8-bit integer. So you can do things like
int key = tolower(word[0]) - 'a';
(gives a value of 0 to 25 if the word starts with a regular "latin" letter). In production code, you'd have to ensure this won't access array elements outside the array (if input does not start with a letter), but it's still so much shorter and more readable than your if-elseif chain.
Also, a hash table works great with a more complex hash function that not just relies on the first element, but distributes words about evenly (but reproducible!) among the lists within the hash table. With this setup, you could determine a reasonable trade-off between memory (table size) and run-time (individual linked list length). With only 26 lists, some of which are much longer than others, your code's performance on large dictionaries has room for improvement.