views:

191

answers:

2

Hi,

This is my first ever attempt at writing a recursive function in c. The following-code works. I apologize for the long posting, but I am trying to be as clear as possible.

I am trying to generate a tree where each node (inode) has an integer field "n". Correspondingly, each inode has an array of pointers to "n" other inodes. Function inode *I = gen_tree(inode *I, int nlevels); generates a tree with random number of inodes at each level. The tree is generated in a depth-first fashion. I have several questions.

(a) Is there a better way to write the function?? Any feedback/suggestions would be appreciated.

(b) Can the tree be generated in a BF fashon?

(c) I->i should have an index in which the tree is traversed. How can I write a function to compute I->i?

(d) I->c should have cumulative-sum of all inodes below a given node. How can I write a function to compute I->c?

Thanks in advance,

~Russ

//.h file:
typedef struct integerNode {
  int n;
  int c;
  int i;
  struct integerNode **nodes;
} inode;
inode *new_inode( int n );
inode *gen_itree( inode *I, int nlevels );


//Constructor:
inode *new_inode( int n ){
    inode *I;
    I = malloc( sizeof (inode ) );
    I->n = n;
    I->nodes = malloc( n * sizeof (inode* ) );
    return (I );
};

//Generating tree with random-number of nodes:
inode *gen_itree( inode *I, int nlevels ){
    int i, next_level, next_n;
    printf( " \n" );
    printf( " I : %p\n", I );
    printf( " ***** nlevels : %d\n", nlevels );
    printf( " *************\n" );
    if ( nlevels == 0 ) {
        printf( " nlevels == 0!\n");
    } else {
        printf( " I->n : %d\n", I->n );
        printf( " *************\n" );
        next_level = nlevels - 1;
        for ( i = 0; i < I->n; i++ ) {
            printf( " I: %p\n",I);
            printf( " adding node number: %d\n", i );
            next_n = 0 + rand( ) % 3;
            I->nodes[i] = new_inode( next_n );
            printf( " I->nodes[%d]->n: %p, %d\n",i, I->nodes[i],next_n);
            I->nodes[i] = gen_itree( I->nodes[i], next_level );
        }
    }
    printf( " *************\n" );
    printf( " returning I : %p\n", I );//This part is unclear to me!
    printf( " *************\n" );
    return (I);
}

//Main.c
int main( int argc, char** argv ){
    inode *I;
    I = new_inode( 2 );
    I = gen_itree(I,3);
    return ( 1 );
}
+1  A: 

First of all. You have no error checking. You have only coded your happy path. Check that your mallocs dont return NULL!!!

if (malloc returned NULL){
          free memory
          exit(error_code)
}

Then

 I->nodes[i] = new_inode( next_n );
 I->nodes[i] = gen_itree( I->nodes[i], next_level );

This part is quite unclear. You could do this

 I->nodes[i] = gen_itree( new_inode( next_n ), next_level );

Same goes here

I = new_inode( 2 );
I = gen_itree(I,3);

could be

 I = gen_itree(new_inode( 2 ),3);

Also, dont forget to free your allocated memory.

As for (d)

unsigned int get_node_count(inode* i){
    unsigned int counter =0;

    if (!i->nodes) return 0;

     //pseudocode
     for each inode* node in i->nodes{
        counter++
        counter+= get_node_count(node);//accumulate node count in child node
     }

      return counter;
Tom
thanks. (a) malloc without check to keep code simple. (b) `I->nodes[i] = gen_itree( new_inode( next_n ), next_level );` that REALLY helps. That was my lack of understanding. (c) counter code helps me understand how to "structure" traversals.
+1  A: 

Everything looks pretty good. I wouldn't put printf's inside the function unless its for debugging purposes.

#define RANGE 3 // this eliminates 'magic constants'

//Generating tree with random-number of nodes:
inode *gen_itree( inode *I, int nlevels ){
        int i, next_level, next_n;

    if ( nlevels ) { // if nlevels != 0
        next_level = nlevels - 1;
        for ( i = 0; i < I->n; i++ ) {
            next_n = rand( ) % RANGE; // no need for a zero
            I->nodes[i] = new_inode( next_n );
            I->nodes[i] = gen_itree( I->nodes[i], next_level );
        }
    }

    return I;
}

That looks better, but I would even go one step further and eliminate some unnecessary local variables, since they are only used once (except for int i).

For (c), this should work:

//This computes the C's for all nodes under this, including this node  
int computeAllCs( inode *I ){
        int i;
        I->c = 0;
        for ( i = 0; i < I->n; i++ )
            I->c += computeAllCs(I->nodes[i]) + 1;
}

Mind you that "all recursive functions can be written iteratively (aka loop)", so you might want to consider the iterative solutions.

Luis B
that looks clean. thanks, russ.