How does this Binary Search Tree look?

I wrote a BST in C a while back and may use it at some point.

```
search_tree tree_make_empty( search_tree tree )
{
if ( tree != NULL )
{
tree_make_empty( tree->left );
tree_make_empty( tree->right );
free( tree );
}
return NULL;
}

tree_position tree_find( CHAR_DATA *target, search_tree tree )
{
if ( tree == NULL )
return NULL;

if ( target hatedata->target_char )
return tree_find( target, tree->left );
else if ( target > tree->hatedata->target_char )
return tree_find( target, tree->right );
else
return tree;
}

search_tree tree_insert( HATE_DATA *hatedata, search_tree tree )
{
if ( tree == NULL )
{
tree = (HATE_NODE * ) malloc( sizeof( HATE_NODE ) );

if ( tree == NULL )
bug( "tree_insert: out of space!" );
else
{
tree->hatedata = hatedata;
tree->left = tree->right = NULL;
}
}
else if ( hatedata->target_char hatedata->target_char )
tree->left = tree_insert( hatedata, tree->left );
else if ( hatedata->target_char > tree->hatedata->target_char )
tree->right = tree_insert( hatedata, tree->right );

return tree;
}

tree_position tree_find_min( search_tree tree )
{
if ( tree == NULL )
return NULL;
else if ( tree->left == NULL )
return tree;
else
return tree_find_min( tree->left );
}

search_tree tree_delete( HATE_DATA *hatedata, search_tree tree )
{
tree_position pos;

if ( tree == NULL )
bug( "tree_delete: not found" );
else if ( hatedata->target_char hatedata->target_char )
tree->left = tree_delete( hatedata, tree->left );
else if ( hatedata->target_char > tree->hatedata->target_char )
tree->right = tree_delete( hatedata, tree->right );
else if ( tree->left && tree->right )
{
pos = tree_find_min( tree->right );
tree->hatedata = pos->hatedata;
tree->right = tree_de

In tree_find and tree_find_min [edit: and even in tree_insert] you're not really gaining anything from using recursion. For example, I think tree_find_min would probably be clearer something like:

tree_position tree_find_min( search_tree tree )
{
   if ( tree == NULL )
      return NULL;
   while (tree->left != NULL)
       tree = tree->left;
   return tree;
}

As a side-benefit, this may also be faster with some compilers. In code like:

HATE_DATA *hatedata;

 /* ... */

 hatedata = (HATE_DATA * ) malloc( sizeof( HATE_DATA ) );

I'd change it to look more like:

hatedata = malloc(sizeof(*hatedata));

The cast accomplishes nothing useful in C, and can cover up the bug of forgetting to #include to get the proper prototype for malloc. Using sizeof(*hatedata) instead of sizeof(HATE_DATA) means that changing the type only requires changing it in one place (where you've defined the variable), instead of everywhere you've done an allocation.

StackExchange Code Review Q#311, answer score: 6