Simplifying a class template

Question

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 <iostream>
#include <string>
#include <stdlib.h>

using namespace std;

template<class T>

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 < theRoot->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 << theRoot->data << endl;
            PrintTree(theRoot->lChildptr); //<< The error is here 
            PrintTree(theRoot->rChildptr);
        }
    }

    public:
    BinaryTree()
    {
        root = NULL;
    }

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

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



int main()
{
    BinaryTree<string> *tree = new BinaryTree<string>();
    BinaryTree<int> *myBT = new BinaryTree<int>();
    BinaryTree<double> *mydouble = new BinaryTree<double>();
    BinaryTree<char> *mychar = new BinaryTree<char>();

    tree->AddItem("Erick");
    myBT->AddItem(7);
    mydouble->AddItem(9.99999);
    mychar->AddItem('@');


    tree->PrintTree();
    myBT->PrintTree();
    mydouble->PrintTree();
    mychar->PrintTree();

    system ( "pause");
}

Do you want to create just one tree and store different types of data in nodes? — Fatih BAKIR
– Fatih BAKIR, Commented Dec 8, 2014 at 22:30
Yes I think that approach will prevent any unforeseen bugs from happening once this template becomes implemented in my friends library program. — EasyE
– EasyE, Commented Dec 8, 2014 at 22:32
Well, I am not sure but at some point you will lose information with that approach about the data you store in that tree, will you only be printing the tree after you insert the nodes? — Fatih BAKIR
– Fatih BAKIR, Commented Dec 8, 2014 at 22:35
Yes This will be used for every transaction that will include only the name address and phone number of the person — EasyE
– EasyE, Commented Dec 8, 2014 at 22:38
Why don't you store structs or objects which can hold the data you mentioned in this tree implementation? — Fatih BAKIR
– Fatih BAKIR, Commented Dec 8, 2014 at 22:43

Community · Accepted Answer · 2017-05-23 11:33:42Z

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 <stdlib.h> is the C header file. For C++, use <cstdlib>.
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 << instead. It is the most idiomatic way of printing an object in C++.
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).

Stack Exchange Network

Simplifying a class template

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Simplifying a class template

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions