cs50 speller : when debugging I found that my while loop in load isn't executing

Question

// Loads dictionary into memory, returning true if successful, else false
bool load(const char *dictionary)
{
// TODO
FILE* dict = fopen(dictionary,"r");

if (dict == NULL)
{
    return false;
}
//initialize table
for (int i = 0; i < N; i++)
{
    table[i] = NULL;
}
//char array
char word[LENGTH + 1];
//read from file

while(fscanf(dict,"%s\n",word) != EOF)
{
    node *new_node = malloc(sizeof(node));
    /* so we know where the end of the linked list is */
    new_node->next = NULL;

    if(new_node == NULL)
    {
        printf("Couldn't malloc new node\n");
        fclose(dict);
        return false;
    }
    /*hash table*/
    strcpy(new_node->word , word);
    hash_value = hash(word);

    if(table[hash_value] == NULL)
    {
        table[hash_value] = new_node;
    }else
    {
        new_node -> next = table[hash_value];
        table[hash_value] = new_node;
    }
    word_count++;
}
fclose(dict);
return true;
}

I've tried everything i can't seem to find whats wrong. Am I assigning my file incorrectly or is my implementation of fscanf wrong ? I tried debugging with debugg50 and i found that my while loop wasn't executing at all.

Don't use trailing spaces (like newline) in your `scanf` formats. It's almost never needed, and can often cause problems. — Some programmer dude, Apr 04 '23 at 16:41
Please show a [mre] so that we don't have to guess what the needed declarations and driver code looks like. — Ted Lyngmo, Apr 04 '23 at 16:41

score 1 · Answer 1 · answered Apr 04 '23 at 19:14

A few issues ...

Don't use fscanf (and don't compare against EOF).
cs50 dictionary files have only one word per line.
Use fgets and then strcspn to remove the newline.
You're dereferencing new_node (with new_node->next = NULL;) before you check it for NULL
If your hash function does not use % N, you should do that in the caller.
Your table setting logic can be simplified. No need to special case a NULL value for table[hash_value]

For some additional help, please see my cs50 speller answer: cs50 pset5 Speller optimisation

Here is the cleaned up code:

// Loads dictionary into memory, returning true if successful, else false
bool
load(const char *dictionary)
{
    // TODO
    FILE *dict = fopen(dictionary, "r");

    if (dict == NULL) {
        return false;
    }

    //initialize table
    for (int i = 0; i < N; i++) {
        table[i] = NULL;
    }

    //char array
    char word[LENGTH + 1];

    //read from file
    while (fgets(word,sizeof(word),dict) != NULL) {
        // strip newline
        word[strcspn(word,"\n")] = 0;

        if (new_node == NULL) {
            printf("Couldn't malloc new node\n");
            fclose(dict);
            return false;
        }

        // copy value into node
        strcpy(new_node->word, word);

        // compute the hash
        hash_value = hash(word);
        hash_value %= N;

        new_node->next = table[hash_value];
        table[hash_value] = new_node;

        word_count++;
    }

    fclose(dict);

    return true;
}

Nicely done. Only other thing I usually mention is how the default `N` is woefully inadequate. So much so you don't really have a hash-table but more a collection of linked lists. And finally how the hash-table *Load-Factor* shouldn't exceed 70% (60% is more ideal) being the number of `buckets filled / total buckets`, or here `words in dict / N` (presuming a decent hash algorithm with few if any collisions for words) — David C. Rankin, Apr 05 '23 at 02:26

cs50 speller : when debugging I found that my while loop in load isn't executing

1 Answers1