1

Lets say I have a class player, which has the member variables num_houses and a pointer temp1 of type house

class player{
private:
    int num_houses;
    house* temp1;
public:
    player(){
    num_houses = 0;
    temp1 = new house[num_houses];
}

My question lies with my code when I try to resize or basically add/remove an element of type house to my array of houses. In my code I happen to be getting a segmentation fault. I'm confused and I am a student so any advice or criticism will help me dearly!

void player::add_house(house tkn){
    house* temp = new house[num_houses];
    for(int i = 0; i < num_houses; i++){
        temp[i] = temp1[i];
    } 
    num_houses++;
    if(temp1 != NULL){
        delete [] temp1;
    }
    temp1 = new house[num_houses];
    for(int i = 0; i < num_houses-1; i++){
        temp1[i] = temp[i];
    }
    temp1[num_houses-1] = tkn;
    delete [] temp;
}
Rob
  • 11
  • 1
  • 3

2 Answers2

1
temp1 = new house[num_houses];

is a problem when num_houses is 0.

Change the default constructor to:

player() : num_houses(0), temp1(nullptr) {}

You can simplify the member function add_house so it requires one less pair of new/delete.

void player::add_house(house tkn){

    num_houses++;
    house* temp = new house[num_houses];
    for(int i = 0; i < num_houses-1; i++){
        temp[i] = temp1[i];
    } 

    temp[num_houses-1] = tkn;

    if(temp1 != nullptr){
        delete [] temp1;
    }

    temp1 = temp;
}
R Sahu
  • 204,454
  • 14
  • 159
  • 270
  • Thanks for the advice, I would also like to add that my copy constructor, destructor and assignment operator overload are all created for my house class. Do you think that there could be a issue with shallow vs deep copy in this situation? I have been getting a "pointer being freed was not allocated" error in my program. – Rob May 09 '18 at 06:07
  • @RobertYelas, most likely. Make sure to follow [The Rule of Three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). – R Sahu May 09 '18 at 06:08
  • small remark, I think that the check _if(temp1 != nullptr)_ we are not needed. We can safely call _delete_ with the value of _null_ptr_ . – Alexcei Shmakov May 09 '18 at 06:29
  • @AlexceiShmakov, The C++11 standard is vague about what happens when the argument to `delete` is a `nullptr`. See https://stackoverflow.com/questions/25329576/calling-delete-on-null-pointers-c03-vs-c11. – R Sahu May 09 '18 at 06:34
  • @RSahu, thank you for the intresting link. I did not know about it. – Alexcei Shmakov May 09 '18 at 06:45
  • Is that actually a problem? I thought allocating zero of something is legal. And what have you fixed in your rewrite? – user253751 May 09 '18 at 06:52
  • @immibis the standard is noncommittal about what happens when the argument to the array form of `new` is zero. – R Sahu May 09 '18 at 06:57
1

For increasing array size rewrite your algo to perform these steps:

  1. Allocate new temp array with size of current+1.
  2. Copy everything from current array to temp array.
  3. Increment houses amount.
  4. delete current array.
  5. Save the pointer to temp array to the current array pointer.
  6. Add new house to the end of current array.

Of course you can reorder this list if you understand how (4 is always after 1, 6 is always after 1, 5 is always after 4 etc.).

P.S. you don't have to check if pointer is not null before deleting it - deleting nullptr won't fail.

UPD: added code

void player::add_house(house tkn {
    house* temp = new house[num_houses + 1];
    for(int i = 0; i < num_houses; i++) {
        temp[i] = temp1[i];
    }
    num_houses++;
    delete [] temp1;
    temp1 = temp;
    temp1[num_houses-1] = tkn;
}