0

I am trying to swap the content in the arrays by swapping the pointers pointing to the two arrays.

My method is the same as what Daniel answered in this question: Swap arrays by using pointers in C++. But the difference is that my array will be a member in a class. My code can be compiled successfully, but the output results are quite weird.

This is my header file:

#include <stdio.h>
#include <iostream>

class Map
{
public:
    Map(int times); // Create an empty map (i.e., one with no key/value pairs)

    int size();     // Return the number of key/value pairs in the map.
    void dump();
    void swap(Map &other);
    int *retrieve();
    void setptr(int *newptr);

private:
    int *ptr;
    int array_1[5];
};

Here is my implementation:

#include "Map.h"
#include <iostream>

using namespace std; 

Map::Map(int times) {
    for (int i = 0; i < 5; i++) {
        array_1[i]=i*times;
    }
    ptr=array_1;
}

void Map::dump() {
    ptr=array_1;
    for (int i = 0; i < 5; i++) {
        cout << *ptr << endl;
        ptr++;
    }
    for (int i = 0; i < 5; i++) {
        ptr--;
    }  
}

void Map::swap(Map &other) {
    int *temp;
    temp = this->ptr;
    this->ptr = other.retrieve();
    other.setptr(temp);
}

int *Map::retrieve() {
    return ptr;
}

void Map::setptr(int *newptr) {
    ptr=newptr;
}

Can anyone tell me what is wrong and how to implement it smartly?

Community
  • 1
  • 1
Hao9000
  • 149
  • 1
  • 1
  • 9

2 Answers2

0

The problem with your design is that the pointer refers to an array in the same object.

Suppose you have to objects a and b. If you swap their pointers, a.ptr will point to b.array_1 which contains the data. reciprocally b.ptr will point to a.array1.

Unfortunately if one of the object -- say b -- gets destroyed (because it was a local object that goes out of scope, or for whatever reason) the pointer of the remaining object would point to an array which doesn't exist anymore. This is UB.

To solve your issue, you'd neet to allocate an array dynamically in the constructor. Get rid of array_1 completely: 

Map::Map(int times){
    ptr=new int[5];  // or better define a constant to avoid hard coded sizes        
    for (int i=0;i<5;i++){
        ptr[i]=i*times;
    }
}

Note that if you use pointers, you need to ensure the invarients on it. This means that you should define also the copy constructor and the assignment operator (to avoid the ptr to be blindly copied), as well as a destructor (to delete the dynamically allocated array).

P.S.: I suppose that you are learning C++ and are not yet familiar with vectors. These would avoid all the hassles here

Edit: if you experience your problem before any object is destroyed, it's because of a bad implementation of dump(): you increment the pointer there in, so that it will no longer point to the start of the array. 

void Map::dump(){
    for (int i=0;i<5;i++){
        cout<<ptr[i]<<endl;  // don't change ptr value !! 
    }
}

One simple trick to avoid such problems, is to systematically declare the member functions that are not supposed to change the state of the object as const: 

class Map {
   ...
   void dump() const;
   ...
}

Then the compiler issues an error if you try to accidentally change a member.

Christophe
  • 68,716
  • 7
  • 72
  • 138
  • Christophe, thank you for your suggestion. But this is a course problem and the instruction doesn't allow us to use dynamically allocated array. Besides, my problem occurred before one of the object was destructed. – Hao9000 Jan 19 '16 at 00:11
  • Ok, I see, I've edited for the other problem in your code ! – Christophe Jan 19 '16 at 00:18
0

The following code runs fine:

#include <stdio.h>
#include <iostream>
#include <conio.h>
using namespace std; 
class Map
{
public:
    Map(int times);         // Create an empty map (i.e., one with no key/value pairs)
    int size();    // Return the number of key/value pairs in the map.
    void dump();
    void swap(int &other);
    int *retrieve();
    void setptr(int *newptr);
private:
    int *ptr;
    int array_1[5];
};


Map::Map(int times){
    for (int i=0;i<5;i++){
        array_1[i]=i*times;
    }
    ptr=array_1;
}

void Map::dump(){
    for (int i=0;i<5;i++)
    {
        cout<<ptr[i]<<endl;        
    }

}

void Map::swap(int &other){
    int *temp;
    temp=this->ptr;
    this->ptr=&other;
    other = *temp;
}

int *Map::retrieve(){
    return ptr;
}

void Map::setptr(int *newptr){
    ptr=newptr;
}

int main()
{
    Map m(2);
    Map n(3);
    m.dump();
    m.swap(*n.retrieve());
    m.dump();
    getchar();
}

1) Added a main function
2) Changed Swap function But the problem that christopher pointed out will still persist i.e the pointer will point to an array in another object.

Edit: You probably need something like this:

void Map::swap(Map &other){
    Map *temp;
    temp=this;
    *this = other;
    other = *temp;
}

Map *Map::retrieve(){
    return this;
}

Note: it is probably not elegant.

novice
  • 545
  • 3
  • 16
  • Thank you for your code. It solved my problem. But my coding instruction requires using exactly the same signature it offers. I have to stick to void swap(Map &other). – Hao9000 Jan 19 '16 at 00:46
  • Is this a homework question? I think what might be required would be to swap the two Map pointers – novice Jan 19 '16 at 00:51
  • Yes, it is homework. Here are the requirements. void swap(Map& other); // Exchange the contents of this map with the other one. The swap function is easily implementable without creating any additional array or additional Map. Do you think swap the Map pointers would be a better solution? – Hao9000 Jan 19 '16 at 01:01
  • Yes, of course otherwise a pointer in one map would point to an array in another map. you have to swap the map pointers. – novice Jan 19 '16 at 01:02
  • are there any other requirements? post all in the question! – novice Jan 19 '16 at 01:09
  • Hi, novice, can you kindly show me how to swap the object pointer? It would be great if you can teach me. – Hao9000 Jan 19 '16 at 04:33