0

So I have created a Binary Search Tree (BST) by placing nodes into a vector. These nodes store 3 values, a user input int ID, a user input int age, and a user string input name.

When inserting these nodes into the vector, they are stored going in ascending order.

Currently I'm working with two nodes.

104 10 Bob

102 11 Steve

When pushing back the first node, there are no problems; however, when attempting to push back the second node, I am getting an out_of_bounds error thrown by the vector class.

I believe something is wrong with my insert function when attempting to switch the positions of these two nodes, however I can't tell exactly where the issue lies.

#include "BinaryTree.h"
#include <string>
#include <iostream>
#include <vector>

using namespace std;
int index;

struct Node
{
    int ID;
    int age;
    string name;

    Node()
    {

    }

    Node(int id, int Age, string nm)
    {
        this->ID = id;
        this->age = Age;
        this->name = nm;
    }
};

vector<Node> binaryTree;


BST::BST()
{

}



void BST::start()
{
    int choice;


    cout << "What would you like to do?" << endl;
    cout << "1. Add a node to the tree" << endl;
    cout << "2. Delete a node from the tree" << endl;
    cout << "3. Find a node in the tree" << endl;
    cout << "4. Report the contents of the tree" << endl;
    cout << "5. Exit program" << endl;

    cin >> choice;

    if (choice == 1)
    {
        insert();
    }

    if (choice == 3)
    {
        find();
    }

    if (choice == 4)
    {
        report();
    }


}


void BST::insert()
{

    int ID;
    int AGE;

    string NAME;

    cout << "Please enter the ID number, age and name" << endl;
    cin >> ID >> AGE >> NAME;

    Node *tree = new Node(ID, AGE, NAME);

    if (index == 0)
    {
        binaryTree.push_back(*tree);
        index++;
    }

    if (index > 0)
    {
        if ((binaryTree.at(index - 1).ID) < ID)
        {
            binaryTree.push_back(*tree);
            index++;
        }
    }


    if (index > 0)
    {
        if ((binaryTree.at(index - 1).ID) > ID)
        {
            Node *temp = new Node();
            *temp = binaryTree.at(index - 1);
            binaryTree.at(index - 1) = *tree;

            binaryTree.at(index) = *temp;
            index++;
        }
    }

    cout << "Added! Size: " << binaryTree.size() << endl;
    cout << " " << endl;
    start();

Would appreciate the help! Thanks!

Granzo
  • 59
  • 1
  • 7
  • Shouldn't your Node hold 2 Nodes? – ZivS May 03 '18 at 06:35
  • @ZivS - It's a representation where the children of `i` are `2*i` and `2*i + 1`. Hence the vector – StoryTeller - Unslander Monica May 03 '18 at 06:38
  • You're leaking memory. You dynamically allocate an object, copy the object into the vector, then lose the original when the function ends. You're also doing something similar in the `if`. Simply getting rid of the pointers and dynamic allocation will fix the memory leaks. – chris May 03 '18 at 06:38
  • If this were using a different data structure it would, but with it being store in an vector, it doesn't exactly "need" it. At least that's what I'm assuming – Granzo May 03 '18 at 06:39
  • 3
    1) Isn't it just typo: `binaryTree.at(index - 1) = *tree; binaryTree.at(index) = *temp;` (note the 2nd `at`)? Since `index` holds the amount of elements in the `vector` (for which I see no point, since `std::vector` has `size` method), the maximum valid index is `index - 1`. 2) You are leaking memory with all of these `new` calls without `delete`ing what you `new`. – Algirdas Preidžius May 03 '18 at 06:40
  • @chris Noted. Thank you. I do plan on doing a bit of clean up after I get this issue solved. – Granzo May 03 '18 at 06:41
  • @Granzo "_I do plan on doing a bit of clean up after I get this issue solved._" Yet, my comment mentioned the reason for the out_of_range being thrown.. – Algirdas Preidžius May 03 '18 at 06:43
  • @AlgirdasPreidžius You posted your comment as I was posting the response to chris. Hence the @. – Granzo May 03 '18 at 06:45
  • You should *never* have `new` appear outside the constructor of a class named "something_ptr". `Node`s are created like `Node tree(ID, AGE, NAME);` – Caleth May 03 '18 at 08:40

2 Answers2

1

Your vector doesn't resizes when you do this: binaryTree.at(index) = *tree;

Do push_back() then try sort

binaryTree.push_back(*tree;)
std::sort(binaryTree.begin(),binaryTree.end(),[](const Node& n1, const Node& n2){//do your comparations});

Or simply use std::set

If you want to work with std::vector without crashing, then your insert() have to look like this:

void BST::insert()
{
    int ID;
    int AGE;

    string NAME;

    cout << "Please enter the ID number, age and name" << endl;
    cin >> ID >> AGE >> NAME;

    //Node *tree = new Node(ID, AGE, NAME); // Don't use new here, there is no need in this
    Node tree(ID, AGE, NAME);

    binaryTree.push_back(tree);
    std::sort(binaryTree.begin(), binaryTree.end(), [](const Node& n1, const Node& n2)
          {
              //compare your nodes here
              return (n1.ID > n2.ID);
          });

    cout << "Added! Size: " << binaryTree.size() << endl;
    cout << " " << endl;
    start();
}

But this won't be a binary tree. You need other data structure to create binary tree, std::vector can't be binary tree. There is a ready solution for you, look at std::set, It inserts elements like you need, you will need to add your custom compare function to std::set and everything will be fine. Here is std::set example for you:

class Node
{
public:
    Node(int id):ID(id){}
    int ID;
};

class NodeComparator
{
public:
    bool operator()(const Node& n1,const Node& n2)
    {
        return n1.ID < n2.ID;
    }
};

int main()
{
    std::set<Node, NodeComparator> set1;
    set1.insert(10);
    set1.insert(8);
    set1.insert(14);
    set1.insert(2);

    return 0;
}

Here is what you need, std::set sorted ascending: enter image description here

Alexey Usachov
  • 1,364
  • 2
  • 8
  • 15
  • So if i'm understanding correctly, I can delete the two (index > 0) statements and replace them with just a singular std::sort(binaryTree.begin(),binaryTree.end(),[](const Node& n1, const Node& n2)? – Granzo May 03 '18 at 06:44
  • Why would you use `new` here? there is no need for that if you de-reference it and push back a copy of it...either hold unique pointers to the nodes in the vector, or just `binaryTree.push_back(Node(ID, AGE, NAME))`. If you have C++11, you can use `binaryTree.emplace_back(ID, AGE, NAME)`. You should not have any `new` or `delete` spread outside resource classes... – ZivS May 03 '18 at 08:45
  • 1
    @ZivS I agree with you, `new` uses because OP did it in his code. Question was about out of range, not about memory allocation. But ok, I have fixed it and improved my answer. – Alexey Usachov May 03 '18 at 08:56
  • @AlexeyUsachov, you suggested for him to use `delete`, that's why I think you should have improved you answer. removed the down vote :) – ZivS May 03 '18 at 09:00
  • @ZivS Thanks! :)) I just wanted to tell him that if he uses `new` then `delete` is necessary in that his context. – Alexey Usachov May 03 '18 at 09:03
  • As another suggestion, maybe to you as well, adding a class for the comparison seems to me like boilerplate. I would have added an `operator<` which `returns make_tuple(n1.ID, n1.AGE, n1.NAME) < make_tuple(n2.ID, n2.AGE, n2.NAME)`. Makes it much easier to read, makes the `set` declaration simple, and also provide a way to make a second and third sort according to the order of the variables in the tuple. EDIT: just noticed the OP only compares ID, so no need for the tuple... – ZivS May 03 '18 at 09:04
  • @ZivS Yes thanks for additional info, this adds fore flexibility to sort, may be OP will be interested at this information. – Alexey Usachov May 03 '18 at 09:10
1

std::vector has methods other than push_back for inserting elements. Specifically, insert takes a position, where the new element is to be inserted. emplace is even better, as you don't even have to create an element to copy into the vector, you just pass the constructor arguments.

You can find the appropriate place to insert at with std::lower_bound.

#include <algorithm>

void BST::insert()
{
    int ID;
    int AGE;
    std::string NAME;

    std::cout << "Please enter the ID number, age and name" << std::endl;
    std::cin >> ID >> AGE >> NAME;

    auto pos = std::lower_bound(binaryTree.begin(), binaryTree.end(), 
        [](const Node& n1, const Node& n2) { return (n1.ID > n2.ID); });

    binaryTree.emplace(pos, ID, AGE, NAME);

    std::cout << "Added! Size: " << binaryTree.size() << endl;
    std::cout << " " << std::endl;
    // start(); // dubious, see below
}

As an aside, your insert method knowing about start is leaking assumptions that you may later want to change. It would be much better to contain that all within start, like:

void BST::start()
{
    std::cout << "What would you like to do?" << std::endl;
    std::cout << "1. Add a node to the tree" << std::endl;
    std::cout << "2. Delete a node from the tree" << std::endl;
    std::cout << "3. Find a node in the tree" << std::endl;
    std::cout << "4. Report the contents of the tree" << std::endl;
    std::cout << "5. Exit program" << std::endl;

    for(int choice; (std::cin >> choice) && (choice != 5);)
    {   
        switch (choice)
        {
        case 1: insert(); break;
        case 3: find(); break;
        case 4: report(); break;
        }
    }
}
Caleth
  • 52,200
  • 2
  • 44
  • 75