Defining a Node in a class and outside of class

Question

How can the difference between Header-File-1 and Header-File-2 affect the program (which is written in C++)?

Which one is the better practice for coding?

Thank you for your responses in advance.

Header-File-1

#ifndef BST 
#define BST

#include<cstdlib>

class BST{

struct Node{

    DataType dataIn;
    Node * rigth;
    Node * left;
};
     public:
         BST();
        ~BST();
         Node * insert(DataType dataIn);
         Node * remove(DataType dataOut);
         Node * search(DataType dataSearch);
         void printTree();
     private:
         Node * root;
} 
#endif

Header-File-2

#ifndef BST 
#define BST

#include<cstdlib>

struct Node{

    DataType dataIn;
    Node * rigth;
    Node * left;
};

class BST{
     public:
         BST();
        ~BST();
         Node * insert(DataType dataIn);
         Node * remove(DataType dataOut);
         Node * search(DataType dataSearch);
         void printTree();
     private:
         Node * root;
} 
#endif

And *"Which one is the better practice for coding?"* is opinion-based, they both could have potential use-cases — UnholySheep, Feb 05 '18 at 18:32
if you make the definition of `Node` public in header1, then there is little difference. If it is private it will be difficult for users of `BST` to use the pointers returned from your methods — 463035818_is_not_an_ai, Feb 05 '18 at 18:32
@UnholySheep "using namespace std; is always bad ". In general or in the header file? — grahamScan, Feb 05 '18 at 18:36
Don't complicate things up. Write an easy and readable code. In my opinion Use `clsss Node` out side `class BST` that is some day maybe you need another sort of `trees` then you you can use the `class Node` you decared outside. — Raindrop7, Feb 05 '18 at 18:36
Unrelated, I'd spend equatable time pondering the public-facing `insert`, `remove`, and `search` functions, all of which expect a `root` as the first argument, where the only initial root they conceptual should use on initial entry from an external call (assuming they're recursive) is already provided by the private `root` member variable. In short, those parameters shouldn't be needed in the public-facing member functions. That design problem is present in *both* headers. — WhozCraig, Feb 05 '18 at 18:38
Putting it in a header is *always* a bad idea, in a .cpp file it's debatable, but I'd avoid it - see here for a more detailed discussion: https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice — UnholySheep, Feb 05 '18 at 18:38

score 1 · Accepted Answer · answered Feb 05 '18 at 18:37

1

In the first header struct Node is private to the class, while in the second header it is a public top-level struct.

This means that Node of the first header is useless: if you were to write something like this

BST::Node *n = bst.search(nullptr, DataType());

the code would not compile:

error: struct BST::Node is private within this context

Making struct Node public would fix this problem. This would be a slight improvement on the second header, because it would demonstrate closeness of Node to BST.

However, this is not ideal, because Node is still exposed to the outside of BST. This is sub-optimal, because the implementation details are visible to the outside of the class. A better approach would be to make Node private in the BST class, and change the API in such a way that Node is never accessed from outside the BST. This can be achieved by using the root instance variable of the BST class.

answered Feb 05 '18 at 18:37

Sergey Kalinichenko

714,442
84
1,110
1,523

Thanks for the answer. You mean I should not return Node * at all since a user can reach the three. Then I can only return true and false. Am I correct? – grahamScan Feb 05 '18 at 18:54
@grahamScan You could return a public iterator class, or follow the pattern used in `std::map` and return a pair that indicates what happened on insertion, and gives you an additional info in case that something has been replaced. Similarly, you could return the old value on remove, etc. – Sergey Kalinichenko Feb 05 '18 at 18:59
Can you give me an example that uses public iterator class? I am not able to find it. When I put Node definition inside of the class, I got error: unknown type name 'Node'. – grahamScan Feb 07 '18 at 22:19

Raindrop7 · Answer 2 · 2018-02-05T18:50:10.150

If your program use only one single sort of trees (in your case: BST) then it is the same in both cases; either declare it inside the class BST or outside and use containment.

If your program deals with different kinds of trees then you should declare it outside the BST to be re-usable eg:

struct Node{
    DataType dataIn;
    Node * rigth;
    Node * left;
};

class rootedBT {
    public:

    private:
       Node* R;
       Node* L;
       T data;
};

class fullBT {
    public:

    private:
       Node* R;
       Node* L;
       T data;
 };

Also using using namespace std in headers is really horrible thing.
Member data of a class are by default private so your class Node is private by default on contrary to a struct.

Defining a Node in a class and outside of class

2 Answers2