Simplifying a class template

I wrote this code over night for a little project that my friend and I have been working on. This class template was created over a year ago during as a class assignment for C++ programming. I recently reviewed this program and I saw that I created multiple instances of my classes in order to print out different data types in the output.

This is not efficient coding at all and I know it can be prone to bugs. However, would anyone consider making abstract classes instead? Please given me an opinion on how I should change this code in order for it to be efficient and reduce the creation of the same class.

```
#include
#include
#include

using namespace std;

template

class BinaryTree
{
struct Node
{
T data;
Node* lChildptr;
Node* rChildptr;

Node(T dataNew)
{
data = dataNew;
lChildptr = NULL;
rChildptr = NULL;
}
};
private:

Node* root;

void Insert(Node newData, Node &theRoot)
{
if(theRoot == NULL)
{
theRoot = newData;
return;
}
else if(newData->data data)
Insert(newData, theRoot->lChildptr);
else
Insert(newData, theRoot->rChildptr);;
}

void insertnode(T item)
{
Node *newData;
newData= new Node(item);

Insert ( newData, root);
}

void PrintTree(Node* &theRoot)
{
if(theRoot)
{
cout data lChildptr); //rChildptr);
}
}

public:
BinaryTree()
{
root = NULL;
}

void AddItem(T item)
{
insertnode(item);
}

void PrintTree()
{
PrintTree(root);
}
};

int main()
{
BinaryTree *tree = new BinaryTree();
BinaryTree *myBT = new BinaryTree();
BinaryTree *mydouble = new BinaryTree();
BinaryTree *mychar = new BinaryTree();

tree->AddItem("Erick");
myBT->AddItem(7);
mydouble->AddItem(9.99999);
m

A few main points you should attempt to improve in this code:

-
Start using nullptr if you can (C++11). It is much better than the old NULL macro.

-
Never "use namespace" in a header file (Not sure if this is a header file? Doesn't seem like...). That would leak the imported declarations to every other place that included the header. Also be conservative about using namespace in a cpp file. Prefer to explicitly declare the things your are using (e.g.: using std::cout;) or don't do any of that and prefix everything with std::. It is not much more typing anyways. Further discussion here.

-
Properly initialize data in constructors by calling the constructors of the sub-objects:

Node(T dataNew)
    : data(dataNew)   // Better would be 'data(std::move(dataNew))' if C++11
    , lChildptr(NULL) // Better 'nullptr' if C++11
    , rChildptr(NULL) // Same here
{ }

-
#include is the C header file. For C++, use `.

-
Why this:

Node *newData;
   newData= new Node(item);

Instead of just: Node * newData = new Node(item);?

-
PrintTree() would be more flexible if it took the output stream as a parameter. Also, consider providing the stream output operator

-
Mind Const Correctness. PrintTree() is not mutating any member variables, so it should be const.

-
Your new allocated instances are never being freed and are leaking in your test code. If you are going to use dynamic memory, make sure to first attempt a smart pointer or a standard container. This will make your life a lot easier and your code a lot more bug-free. Side note: These local variables you've declared in main() have no reason for being pointers. They could very well be declared by value in the program stack.

-
A little inconsistency with naming: insertnode() is not following your PascalCase notation. As a side note, camelCase is more popular for method names (except for Windows-centric projects perhaps).

StackExchange Code Review Q#72078, answer score: 2