Return to Answer

A few notes:

• typedef struct names are commonly written in PascalCase today.

   Node* current = NULL;
  

• Initialize your variables on the lowest scope as possible. Therefore, declare your variables within your for loops. ^(C99)

   for(int c = fgetc(dict); c != '\n'; c = fgetc(dict)) //iterate through word
  

  Also, I feel as if you should be checking c for EOF, and then check for \n within the for loop.

• The statements inside of your for loop look fishy to me:

   if(current->children[c-'a'] == NULL)
   {
   current->children[c-'a'] = calloc(27,sizeof(node))
   current = current->children[c-'a'];
   }
   else
   { 
   current = current->children[c-'a'];
   } 
  

  No matter what, you are going to set children equal to current->children[c-'a'], so you can move that statement out of both branches of the conditional (DRY). Also, you should not be allocating 27 elements for every single Node. Here is how I would write it:

   if (!current->children[c-'a']) current->children[c-'a'] = calloc(1, sizeof(node));
   current = current->children[c-'a'];
  

• You can remove the != NULL part in your if conditional statements.

• Avoid global variables: root and nwords.

• Avoid magic numbers.

   for (int i = 0; i < 27; i++)
  

  Use a #define instead at the top of your code.

   #define NUM_NODES 27
  

  But where does the number 27 even come from? Documentation would go a long way here to help developers understand what it is.

• Set your Node values to NULL after you free them. This is so that you do not try to access the data again after it has been freed.

A few notes:

• typedef struct names are commonly written in PascalCase today.

   Node* current = NULL;
  

• Initialize your variables on the lowest scope as possible. Therefore, declare your variables within your for loops. ^(C99)

   for(int c = fgetc(dict); c != '\n'; c = fgetc(dict)) //iterate through word
  

  Also, I feel as if you should be checking c for EOF, and then check for \n within the for loop.

• The statements inside of your for loop look fishy to me:

   if(current->children[c-'a'] == NULL)
   {
   current->children[c-'a'] = calloc(27,sizeof(node))
   current = current->children[c-'a'];
   }
   else
   { 
   current = current->children[c-'a'];
   } 
  

  No matter what, you are going to set children equal to current->children[c-'a'], so you can move that statement out of both branches of the conditional (DRY). Also, you should not be allocating 27 elements for every single Node. Here is how I would write it:

   if (!current->children[c-'a']) current->children[c-'a'] = calloc(1, sizeof(node));
   current = current->children[c-'a'];
  

• You can remove the != NULL part in your if conditional statements.

• Avoid global variables: root and nwords.

• Avoid magic numbers.

   for (int i = 0; i < 27; i++)
  

  Use a #define instead at the top of your code.

   #define NUM_NODES 27
  

  But where does the number 27 even come from? Documentation would go a long way here to help developers understand what it is.

• Set your Node values to NULL after you free them. This is so that you do not try to access the data again after it has been freed.

A few notes:

• typedef struct names are commonly written in PascalCase today.

   Node* current = NULL;
  

• Initialize your variables on the lowest scope as possible. Therefore, declare your variables within your for loops. ^(C99)

   for(int c = fgetc(dict); c != '\n'; c = fgetc(dict)) //iterate through word
  

  Also, I feel as if you should be checking c for EOF, and then check for \n within the for loop.

• The statements inside of your for loop look fishy to me:

   if(current->children[c-'a'] == NULL)
   {
   current->children[c-'a'] = calloc(27,sizeof(node))
   current = current->children[c-'a'];
   }
   else
   { 
   current = current->children[c-'a'];
   } 
  

  No matter what, you are going to set children equal to current->children[c-'a'], so you can move that statement out of both branches of the conditional (DRY). Here is how I would write it:

   if (!current->children[c-'a']) current->children[c-'a'] = calloc(27,sizeof(node));
   current = current->children[c-'a'];
  

• You can remove the != NULL part in your if conditional statements.

• Avoid global variables: root and nwords.

• Avoid magic numbers.

   for (int i = 0; i < 27; i++)
  

  Use a #define instead at the top of your code.

   #define NUM_NODES 27
  

  But where does the number 27 even come from? Documentation would go a long way here to help developers understand what it is.

• Set your Node values to NULL after you free them. This is so that you do not try to access the data again after it has been freed.

A few notes:

• typedef struct names are commonly written in PascalCase today.

   Node* current = NULL;
  

• Initialize your variables on the lowest scope as possible. Therefore, declare your variables within your for loops. ^(C99)

   for(int c = fgetc(dict); c != '\n'; c = fgetc(dict)) //iterate through word
  

  Also, I feel as if you should be checking c for EOF, and then check for \n within the for loop.

• The statements inside of your for loop look fishy to me:

   if(current->children[c-'a'] == NULL)
   {
   current->children[c-'a'] = calloc(27,sizeof(node))
   current = current->children[c-'a'];
   }
   else
   { 
   current = current->children[c-'a'];
   } 
  

  No matter what, you are going to set children equal to current->children[c-'a'], so you can move that statement out of both branches of the conditional (DRY). Also, you should not be allocating 27 elements for every single Node. Here is how I would write it:

   if (!current->children[c-'a']) current->children[c-'a'] = calloc(1,sizeof(node));
   current = current->children[c-'a'];
  

• You can remove the != NULL part in your if conditional statements.

• Avoid global variables: root and nwords.

• Avoid magic numbers.

   for (int i = 0; i < 27; i++)
  

  Use a #define instead at the top of your code.

   #define NUM_NODES 27
  

  But where does the number 27 even come from? Documentation would go a long way here to help developers understand what it is.

• Set your Node values to NULL after you free them. This is so that you do not try to access the data again after it has been freed.

A few notes:

• typedef struct names are commonly written in PascalCase today.

   Node* current = NULL;
  

• Initialize your variables on the lowest scope as possible. Therefore, declare your variables within your for loops. ^(C99)

   for(int c = fgetc(dict); c != '\n'; c = fgetc(dict)) //iterate through word
  

  Also, I feel as if you should be checking c for EOF, and then check for \n within the for loop.

• The statements inside of your for loop look fishy to me:

   if(current->children[c-'a'] == NULL)
   {
   current->children[c-'a'] = calloc(27,sizeof(node))
   current = current->children[c-'a'];
   }
   else
   { 
   current = current->children[c-'a'];
   } 
  

  No matter what, you are going to set children equal to current->children[c-'a'], so you can move that statement out of both branches of the conditional (DRY). Here is how I would write it:

   if (current->children[c-'a']) current->children[c-'a'] = calloc(27,sizeof(node));
   current = current->children[c-'a'];
  

• You can remove the != NULL part in your if conditional statements.

• Avoid global variables: root and nwords.

• Avoid magic numbers.

   for (int i = 0; i < 27; i++)
  

  Use a #define instead at the top of your code.

   #define NUM_NODES 27
  

  But where does the number 27 even come from? Documentation would go a long way here to help developers understand what it is.

• Set your Node values to NULL after you free them. This is so that you do not try to access the data again after it has been freed.

A few notes:

• typedef struct names are commonly written in PascalCase today.

   Node* current = NULL;
  

• Initialize your variables on the lowest scope as possible. Therefore, declare your variables within your for loops. ^(C99)

   for(int c = fgetc(dict); c != '\n'; c = fgetc(dict)) //iterate through word
  

  Also, I feel as if you should be checking c for EOF, and then check for \n within the for loop.

• The statements inside of your for loop look fishy to me:

   if(current->children[c-'a'] == NULL)
   {
   current->children[c-'a'] = calloc(27,sizeof(node))
   current = current->children[c-'a'];
   }
   else
   { 
   current = current->children[c-'a'];
   } 
  

  No matter what, you are going to set children equal to current->children[c-'a'], so you can move that statement out of both branches of the conditional (DRY). Here is how I would write it:

   if (!current->children[c-'a']) current->children[c-'a'] = calloc(27,sizeof(node));
   current = current->children[c-'a'];
  

• You can remove the != NULL part in your if conditional statements.

• Avoid global variables: root and nwords.

• Avoid magic numbers.

   for (int i = 0; i < 27; i++)
  

  Use a #define instead at the top of your code.

   #define NUM_NODES 27
  

  But where does the number 27 even come from? Documentation would go a long way here to help developers understand what it is.

• Set your Node values to NULL after you free them. This is so that you do not try to access the data again after it has been freed.