Delete data from a dynamically allocated array in c++

Question

I am trying to delete a record from an entered position, from a dynamically allocated array in c++, and when i try to run the program it throws an error stating

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc

The insertion and displaying of the records are running perfectly fine, the only thing that throws an error is delete operation.

The Code

#include <iostream>
using namespace std;

struct employee{
    string name;
    int empId;
    string dept;
    int age;
};

employee *emp = new employee[5];

void insertData(){
    for (int i = 0; i<5; i++){
        cout<<"Enter the Employee name"<<endl;
        cin>>emp -> name;
        cout<<"Enter the Employee Id"<<endl;
        cin>>emp -> empId;
        cout<<"Enter the Employee Department"<<endl;
        cin>>emp -> dept;
        cout<<"Enter the Employee age"<<endl;
        cin>>emp -> age;
    }
}

void displayData(){
    for (int i = 0; i < 5; ++i) {
        cout<<"Employee"<<i+1<<" Data"<<endl;
        cout<<"Name : "<<emp -> name<<endl;
        cout<<" Employe ID : "<<emp -> empId<<endl;
        cout<<"Department : "<<emp -> dept<<endl;
        cout<<"Age : "<<emp -> age<<endl<<endl;
    }
}

void deleteData(){
    int pos;
    cout<<"Enter the position you want to delete Data";
    cin>>pos;
    if (pos>5){
        cout<<"Invalid Size please enter a size smaller than 5";
    }
    for (int i = pos; i < 5; ++i) {
        emp[i] = emp[i+1];
    }
}

int menu(){
    int x;
    do {
        int n;
        cout << "Please enter the number corresponding to an operation you want to perform\n";
        cout << "1. Insert Data" << endl;
        cout << "2. Display Data" << endl;
        cout << "3. Delete Data" << endl;
        cout << "4. Exit" << endl;
        cin >> n;

        switch (n) {

            case 1: {
                insertData();
                break;
            }
            case 2: {
                displayData();
                break;
            }
            case 3: {
                deleteData();
                break;
            }
            case 4: {
                exit(0);
            }
            default:
                cout << "Invalid Choice, Enter a valid choice";
                return 1;
        }
        cout<<"Press 1 to continue or 0 to exit";
        cin>>x;
    } while (x == 1);
}

int main() {

    menu();
    return 0;
}

Answer 1

Your code has several logic issues.

Inserting and displaying data

When you insert and display data with insertData and displayData you loop over five indices (i) but you never used that variable in your loop. You simply operate all five times on the pointer to the first element in the array.

Deleting data

You print an error message if pos is greater than 5, but then you go ahead and run the rest of the function anyway. A return would break out of the function at this point. Also, I suggest writing to std::cerr for error messages.

You iterate from pos to 4, but copy from index 5, which is out of bounds. This yields undefined behavior. You should change your bounds to i < 4. Because arrays are indexed starting at 0 you also need to start at pos - 1.

You never clear out the data in the last element. As you're only deleting one record at a time, you now it'll always be the last one that needs to be cleared.

void deleteData(){
    int pos;
    cout<<"Enter the position you want to delete Data";
    cin>>pos;
    if (pos>5){
        cout<<"Invalid Size please enter a size smaller than 5";
    }
    for (int i = pos; i < 5; ++i) {
        emp[i] = emp[i+1];
    }
}

Suggested:

void deleteData() {
    int pos;
    cout << "Enter the position you want to delete Data ";
    cin >> pos;

    if (pos > 5){
        cout << "Invalid Size please enter a size smaller than 5\n";
        return;
    }

    for (int i = pos - 1; i < 4; ++i) {
        emp[i] = emp[i+1];
    }

    emp[4].name = "";
    emp[4].empId = 0;
    emp[4].dept = "";
    emp[4].age = 0;
}

Menu

In your menu function you should initialize x to 1, as you don't error check user input at the end.

You return in the default case, which will prevent the loop from repeating to get a valid input.

You never use the return value of menu and it may not ever return, which your compiler should warn you about. The return type should be void.

void menu() {
    int x = 1;
    do {
        int n;
        cout << "Please enter the number corresponding to an operation you want to perform\n";
        cout << "1. Insert Data" << endl;
        cout << "2. Display Data" << endl;
        cout << "3. Delete Data" << endl;
        cout << "4. Exit" << endl;
        cin >> n;

        switch (n) {
            case 1: 
            insertData();
            break;
            
            case 2: 
            displayData();
            break;
            
            case 3: 
            deleteData();
            break;
            
            case 4: 
            exit(0);
            
            default:
            cerr << "Invalid Choice, Enter a valid choice";
        }

        cout << "Press 1 to continue or 0 to exit";
        cin >> x;
    } while (x == 1);
}

Best practices

You use the magic number 5 a lot. Give it a name so it's easier to modify if needed.

const int NUM_EMPLOYEES = 5;

There are many suggestions. Your array does not have to live at a global scope. It should not. It should be declared inside main and then passed to the functions as an argument.

Further, there is no need for it to be dynamically allocated at all. If you are going to dynamically allocate with new you'll want to remember to de-allocate with delete []. Most modern desktops OSes will automatically release that memory when your program finishes, but it's a good habit to get into.

Alternatively, you can dynamically allocate with a smart pointer and the smart pointer will handle the de-allocation for you.

auto emp = std::make_unique<emp[]>(NUM_EMPLOYEES);

Incorporating some of these ideas might look like:

int main() {
    employee emp[NUM_EMPLOYEES];

    insertData(emp, NUM_EMPLOYEES);

    return 0;
}

You tie modifying your data very closely to the UI. These can and probably should be separated. For instance, you might have a int menu() function that prompts for a menu choice and returns it, but doesn't actually do anything to the data.

int menu() {
    while (true) {
        cout << "Please enter the number corresponding to an operation you want to perform\n";
        cout << "1. Insert Data" << endl;
        cout << "2. Display Data" << endl;
        cout << "3. Delete Data" << endl;
        cout << "4. Exit" << endl;

        int choice = 0;
        cin >> choice;

        if (choice < 1 || choice > 4) {
           cerr << "Invalid option." << endl;
           continue;
        }

        return choice;
    }
}

You can now use this valid menu choice to decide what to do to your data.

Because all of your functions need to accept this data, you should make your collection of employees a class/struct, and implement these functions as member functions. I suspect that's a bit beyond where you're at with C++, but object oriented programming and model-view-controller design are things to read up on.

Answer 2

This (out of scope) memory allocation is so wrong...

employee *emp = new employee[5]; 

just do it:

employee emp[5];

In your deleteData() perhaps you want to set entries to zero instead of copying from the next position (I guess this is what delete denotes). For example you may want this implementation:

void deleteData(){
    int pos;
    cout<<"Enter the position you want to delete Data";
    cin>>pos;
    if (pos<5) {
        emp[pos].name = "";
        emp[pos].empId = 0;
        emp[pos].dept = "";
        emp[pos].age = 0;
    } else {
        cout<<"Invalid Size please enter a size smaller than 5";
    }
}

This implementation prevents the potential out-of-bound errors!