Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to create a multiway tree with the following code in C++. As of now, it is more like a sample piece of code.

I wish to do the following with this piece of code(and I am able to get the desired result):

  1. Create a struct Node of two members : value(of the node) and array of children of the node.
  2. Make 'root's value = 5
  3. Make the children's values equal to 10,20,30 & 40
  4. Make the children of children's values equal to null(=0);

I am getting the desired output , but what I want to know is if I am doing it the right way.

Thanks, in advance.

//multinode tree
#include<iostream>
#define null 0

struct Node
{
    int value;
    Node *child[4]; 
};

void display(Node *root){
    if(root==null)
        return;
    else{

        std::cout<<root->value<<" ";
        if(root->child[0]!=null)
            for (int i=0;i<4;i++)
            {
                if(root->child[i]!=null)
                    display(root->child[i]);

            }
    }
}

int main()
{
    Node *root=new Node;
    Node *test=null;
    root->value=5;
    int values[]={10,20,30,40};
    int i=0;
    for (i=0;i<4;i++)
    {
        Node *temp=new Node;
        temp->value=values[i];
        root->child[i]=temp;
        for (int j=0;j<4;j++)
        {
            root->child[i]->child[j]=null;
        }
        std::cout<<"Root value "<<root->child[i]->value<<"\n\n ";
    }
    std::cout<<"\n\n\n\n\n";
    display(root);

}
share|improve this question
    
I've fixed the indentation on this code. There was some non-standard indentation that could confuse readers. –  sharth Feb 7 '13 at 1:22
    
Thanks you for that. –  Rahul Rajaram Feb 7 '13 at 3:47
    
Also, "#define null 0"? What's wrong with NULL? –  Johnsyweb Feb 7 '13 at 8:21
add comment

migrated from stackoverflow.com Feb 7 '13 at 12:52

This question came from our site for professional and enthusiast programmers.

2 Answers

This is a very C like approach and you are using C++ so take advantage of that fact.

  1. Get rid of the #define and use nullptr keyword instead.

  2. Name member variables something special such as m_ so Node *m_children[4];

  3. Try use C++ data structures instead of arrays and char* so vector<Node*> children.

  4. Make the Node struct into a class and write getters and setters to encapsulate the new vector structure you should use.

  5. Encapsulate the void display(Node *root) function into a class such:

    class Tree
    {
        Tree(int depth);// create a tree with x depth
        etc...
        void display();
    public:
        Node* m_root;
    };
    

    You could create some functionality to able to initialise the tree with some custom data structure.

  6. Add the following code in main into the tree class

    for (i=0;i<4;i++)
    {
        Node *temp=new Node;
        temp->value=values[i];
        root->child[i]=temp;
        for (int j=0;j<4;j++)
        {
            root->child[i]->child[j]=null;
        }
        std::cout<<"Root value "<<root->child[i]->value<<"\n\n ";
    }
    

PS. Sorry if the code doesn't work; I'm at work and have no way to check it!

share|improve this answer
    
Additionally you might want to add into the Node struct another Node* to its parent so you can reverse navigate the tree also. –  Chris Condy Feb 7 '13 at 1:46
3  
#2 is a style choice, not necessarily C++ mandated.. –  Karthik T Feb 7 '13 at 1:49
    
#2 devx.com/cplus/10MinuteSolution/35167 open-std.org/jtc1/sc22/wg21/docs/papers/2003/n1488.pdf Its a bad approach if you are using C++ instead of C. There is a keyword for very good reasons. –  Chris Condy Feb 7 '13 at 2:53
    
I was talking about the m_ prefix, I have nothing against nullptr –  Karthik T Feb 7 '13 at 3:07
    
Sorry, my mistake yes your right it is a style choice. But any similar style choice is good. I'm just highly recommending for the same reason we dont call everything "a", "b", "c" or writing our code on 1 line or just having 1 file with everything. –  Chris Condy Feb 7 '13 at 3:10
show 1 more comment

This is correct enough.

You could improve it in a couple of ways,

  • if you do new Node(), note the additional (), its members will be zero initialized, so you can skip the nulling of the children.
  • you can add a constructor which initializes among other things the arrays to null, so that you don't need to remember to do it every time, Recommended for readability and future proof as commenters suggest?
share|improve this answer
1  
The standard does not require default initialisation of members, so your first advice is dangerous. –  Cong Xu Feb 7 '13 at 1:28
1  
@CongXu I am going based on Do the parentheses after the type name make a difference with new? –  Karthik T Feb 7 '13 at 1:31
    
@KarthikT: I think it's dangerous to rely on that behavior. What happens if someone unwittingly adds a new member to the Node class which makes it non-POD? –  Nik Bougalis Feb 7 '13 at 1:40
    
@NikBougalis correct me if I am wrong, but it looks to me like the link promises 0 initialization as long as there is no user defined constructor. Regardless, I would go the explicit route (#2) myself. –  Karthik T Feb 7 '13 at 1:45
1  
@RahulRajaram imo, the right way is that they be null by default instead of having to do it manually, safer that way, you cant forget. –  Karthik T Feb 7 '13 at 4:48
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.