Return to Answer

"whether its bad practice or not" to sub-divide. No, not bad practice - in fact good in certain areas. It often comes down to maintenance - how many functions per file and the optimal answer is highly problem dependent. IMO, too often files need fewer functions than code needs fewer files.

ungetch() have troubles as it ungets a EOF. This special value should not be unget-able. Further – I have doubts that when char is signed, that these 2 functions properly handle some char values as the real getc() returns unsigned char and EOF. Example: If (char) 255 is un-gotten, on get, it may have the value of EOF. Recommend making your unget buffer unsigned char.

Unclear to the whole need for user unget/get functionality as standard library ones well handle 1 level of get/unget.

The whole addtree(), should input be alphabetical, devolves into a linked list. Consider AVL trees.

Below test i against size after the assignment. Recommend test before to cope with pathological size values like 1. Recommend re-write - avoiding code like (word+i++). Further, consider size_t rather than int for array indexes.

// from
do {
 *(word+i++) = c;
} while (isalpha(c = getch()) && i < size-1);
// to
if (size > 0) size--; 
while (i < size && isalpha(c)) {
 word[i++] = c;
 c = getch()
};

MAX_WORD is fuzzy. Suggest MAX_WORD_SIZE.

ungetch() and getch() look way too much like standard functions. Suggest io_ungetch() and io_getch().

Cast not needed.

 // return (struct tnode *) malloc(sizeof(struct tnode));
 return malloc(sizeof(struct tnode));
 // p = (char *) malloc(strlen(s)+1);
 p = malloc(strlen(s)+1);

Magic number 4. Why 4?

 // printf("%4d %s\n", node->count, node->word);
 printf("%d %s\n", node->count, node->word);

If a function does not modify a pointer's contents, use const.

 // void treeprint(struct tnode *node, int min_count, int max_count)
 void treeprint(const struct tnode *node, int min_count, int max_count)

Minor: .h file void ungetch(int); --> void ungetch(int ch); Also add a little documentation of the .h files, it goes a long way in understanding.

Minor: spelling: doesnt --> doesn't

Pedantic point: char may be signed and is...() defined for unsigned char and EOF, not signed char.

 // if(isalpha(word[0]))
 if(isalpha((unsigned char) word[0]))

"whether its bad practice or not" to sub-divide. No, not bad practice - in fact good in certain areas. It often comes down to maintenance - how many functions per file and the optimal answer is highly problem dependent. IMO, too often files need fewer functions than code needs fewer files.

ungetch() have troubles as it ungets a EOF. This special value should not be unget-able. Further – I have doubts that when char is signed, that these 2 functions properly handle some char values as the real getc() returns unsigned char and EOF. Example: If (char) 255 is un-gotten, on get, it may have the value of EOF. Recommend making your unget buffer unsigned char.

Unclear to the whole need for user unget/get functionality as standard library ones well handle 1 level of get/unget.

The whole addtree(), should input be alphabetical, devolves into a linked list. Consider AVL trees.

Below test i against size after the assignment. Recommend test before to cope with pathological size values like 1. Recommend re-write - avoiding code like (word+i++). Further, consider size_t rather than int for array indexes.

// from
do {
 *(word+i++) = c;
} while (isalpha(c = getch()) && i < size-1);
// to
if (size > 0) size--; 
while (i < size) {
 word[i++] = c;
 c = getch()
 if (!isalpha(c)) {
 break;
 }
}

MAX_WORD is fuzzy. Suggest MAX_WORD_SIZE.

ungetch() and getch() look way too much like standard functions. Suggest io_ungetch() and io_getch().

Cast not needed.

 // return (struct tnode *) malloc(sizeof(struct tnode));
 return malloc(sizeof(struct tnode));
 // p = (char *) malloc(strlen(s)+1);
 p = malloc(strlen(s)+1);

Magic number 4. Why 4?

 // printf("%4d %s\n", node->count, node->word);
 printf("%d %s\n", node->count, node->word);

If a function does not modify a pointer's contents, use const.

 // void treeprint(struct tnode *node, int min_count, int max_count)
 void treeprint(const struct tnode *node, int min_count, int max_count)

Minor: .h file void ungetch(int); --> void ungetch(int ch); Also add a little documentation of the .h files, it goes a long way in understanding.

Minor: spelling: doesnt --> doesn't

Pedantic point: char may be signed and is...() defined for unsigned char and EOF, not signed char.

 // if(isalpha(word[0]))
 if(isalpha((unsigned char) word[0]))

"whether its bad practice or not" to sub-divide. No, not bad practice - in fact good in certain areas. It often comes down to maintenance - how many functions per file and the optimal answer is highly problem dependent. IMO, too often files need fewer functions than code needs fewer files.

ungetch() have troubles as if ungets a EOF. This special value should not be unget-able. Further – I have doubts that when char is signed, that these 2 functions properly handle some char values as the real getc() returns unsigned char and EOF. Recommend making your unget buffer unsigned char.

The whole addtree(), should input be alphabetical, devolves into a linked list. Consider AVL trees.

Below test i against size after the assignment. Recommend test before to cope with pathological size values like 1. Recommend re-write - avoiding code like (word+i++). Further, consider size_t rather than int for array indexes.

// from
do {
 *(word+i++) = c;
} while (isalpha(c = getch()) && i < size-1);
// to
if (size > 0) size--; 
while (i < size && isalpha(c)) {
 word[i++] = c;
 c = getch()
};

MAX_WORD is fuzzy. Suggest MAX_WORD_SIZE.

ungetch() and getch() look way too much like standard functions. Suggest io_ungetch() and io_getch().

Cast not needed.

 // return (struct tnode *) malloc(sizeof(struct tnode));
 return malloc(sizeof(struct tnode));
 // p = (char *) malloc(strlen(s)+1);
 p = malloc(strlen(s)+1);

Magic number 4. Why 4?

 // printf("%4d %s\n", node->count, node->word);
 printf("%d %s\n", node->count, node->word);

If a function does not modify a pointer's contents, use const.

 // void treeprint(struct tnode *node, int min_count, int max_count)
 void treeprint(const struct tnode *node, int min_count, int max_count)

Minor: .h file void ungetch(int); --> void ungetch(int ch); Also add a little documentation of the .h files, it goes a long way in understanding.

Minor: spelling: doesnt --> doesn't

Pedantic point: char may be signed and is...() defined for unsigned char and EOF, not signed char.

 // if(isalpha(word[0]))
 if(isalpha((unsigned char) word[0]))

"whether its bad practice or not" to sub-divide. No, not bad practice - in fact good in certain areas. It often comes down to maintenance - how many functions per file and the optimal answer is highly problem dependent. IMO, too often files need fewer functions than code needs fewer files.

ungetch() have troubles as it ungets a EOF. This special value should not be unget-able. Further – I have doubts that when char is signed, that these 2 functions properly handle some char values as the real getc() returns unsigned char and EOF. Example: If (char) 255 is un-gotten, on get, it may have the value of EOF. Recommend making your unget buffer unsigned char.

Unclear to the whole need for user unget/get functionality as standard library ones well handle 1 level of get/unget.

The whole addtree(), should input be alphabetical, devolves into a linked list. Consider AVL trees.

Below test i against size after the assignment. Recommend test before to cope with pathological size values like 1. Recommend re-write - avoiding code like (word+i++). Further, consider size_t rather than int for array indexes.

// from
do {
 *(word+i++) = c;
} while (isalpha(c = getch()) && i < size-1);
// to
if (size > 0) size--; 
while (i < size && isalpha(c)) {
 word[i++] = c;
 c = getch()
};

MAX_WORD is fuzzy. Suggest MAX_WORD_SIZE.

ungetch() and getch() look way too much like standard functions. Suggest io_ungetch() and io_getch().

Cast not needed.

 // return (struct tnode *) malloc(sizeof(struct tnode));
 return malloc(sizeof(struct tnode));
 // p = (char *) malloc(strlen(s)+1);
 p = malloc(strlen(s)+1);

Magic number 4. Why 4?

 // printf("%4d %s\n", node->count, node->word);
 printf("%d %s\n", node->count, node->word);

If a function does not modify a pointer's contents, use const.

 // void treeprint(struct tnode *node, int min_count, int max_count)
 void treeprint(const struct tnode *node, int min_count, int max_count)

Minor: .h file void ungetch(int); --> void ungetch(int ch); Also add a little documentation of the .h files, it goes a long way in understanding.

Minor: spelling: doesnt --> doesn't

Pedantic point: char may be signed and is...() defined for unsigned char and EOF, not signed char.

 // if(isalpha(word[0]))
 if(isalpha((unsigned char) word[0]))