0

Can't access a 2D array that I created in the constructor in the printMatrix class function. When I call the function in main with a simple cout << "test"; it will print that. If I try to print the values of matrixArray[][] it prints nothing and quits the program. Am I not referencing the 2d array correctly?

class matrix
{
    int **matrixArray;

public:
    int matrixSize = 0;
    matrix(int matrixSize);
    void printMatrix();
    void makeMagicSquare();
};
matrix::matrix(int const matrixSize)
{
    this->matrixSize = matrixSize ;


    int** matrixArray = new int*[matrixSize];
        for(int i = 0; i<matrixSize; i++){
           matrixArray[i] = new int[matrixSize];
        }


    for(int row = 0; row < matrixSize ;row++)
            {
                for(int col = 0; col < matrixSize; col++)
                {
                    matrixArray[row][col] =0;
                    cout << matrixArray[row][col] << "  ";
                }//End for Col
                cout << endl;
            }//End for Row

}
//printMatrix Function 
void matrix::printMatrix(){

for(int row = 0; row < matrixSize;row++)
        {
            for(int col = 0; col < matrixSize; col++)
            {
                cout << "test" << "  ";
                //Not able to print  from print function
                cout << matrixArray[row][col] << endl;
            }// end col
            cout << endl;
        }//end row

}
gsamaras
  • 71,951
  • 46
  • 188
  • 305
PixelPusher
  • 107
  • 10
  • You really should store the matrix in a one dimensional vector. – NathanOliver Sep 21 '16 at 19:54
  • will somebody smack you if you use `vector >` instead? – Jean-François Fabre Sep 21 '16 at 19:54
  • @Jean-FrançoisFabre Only someone who cares a lot about cache locality. 1D vectors are better. – vsoftco Sep 21 '16 at 19:55
  • @vsoftco right about that. I did not see your comment :) I was talking about `int **` versus `vector` and all the memory problems it solves. I agree that 1D vectors are better, but you have to wrap them into an object or you constantly shoot yourself in the foot with index computations. – Jean-François Fabre Sep 21 '16 at 19:56
  • @Jean-FrançoisFabre Yes I knew you were relating to `int**` :) Right about the indexing, but if you don't want to complicate (using a proxy class) one can re-define `operator()(size_t, size_t)` for indexing. – vsoftco Sep 21 '16 at 19:57
  • Yeah the OP is not sure about how to do what he wants, let's bomb him with the vector VS array, 1D vs 2D stuff..One step at a time guys! :) – gsamaras Sep 21 '16 at 19:58
  • don't forget to use SSE2 for vectorized complex multiplications :) – Jean-François Fabre Sep 21 '16 at 19:58
  • isn't there some templates sources like Template Numerical Toolkit to define vectors & matrices without coding something that crashes? – Jean-François Fabre Sep 21 '16 at 20:03
  • @Jean-FrançoisFabre [Eigen](http://eigen.tuxfamily.org/index.php?title=Main_Page) is a VERY good/fast templated C++ matrix library. – vsoftco Sep 21 '16 at 20:17

2 Answers2

4

How could you?

int** matrixArray = new int*[matrixSize];

is a local variable inside the constructor, thus matrixArray is only visible inside the constructor.

Use the data member instead and you will be fine, and since you already have defined that member, you should just do this:

matrixArray = new int*[matrixSize];

Don't forget to delete your memory in the destructor! ;)

gsamaras
  • 71,951
  • 46
  • 188
  • 305
1

You've made a very common mistake, failing to distinguish between locally scoped variables/parameters and member variables.

int** matrixArray = new int*[matrixSize];

Because this expression starts with a type, it is a declaration of a new, local variable, which is "hiding" the class member (you would have to use this->matrixArray to access it, and it would not be initialized). Your compiler should be giving you a "shadow variable" warning.

A common practice is to give member variables a distinguisher: some people prefix them with "m_", some put a trailing "_", so your code would become:

class matrix
{
    int **matrixArray_ = nullptr;  //1
    int matrixSize_ = 0;  //2

public:
    matrix(int matrixSize);
    ~matrix();  //7
    void printMatrix() const;
    void makeMagicSquare();
    int size() const { return matrixSize_; }  //3
};

matrix::matrix(int const matrixSize)
    : matrixSize_(matrixSize)  //4
{
    matrixArray_ = new int*[matrixSize_];  //5
    for(int i = 0; i<matrixSize_; i++) {
        matrixArray_[i] = new int[matrixSize_] ();  //6
    }
}

matrix::~matrix()  //7
{
    if (!_matrixArray)
        return;
    for (int i = 0; i < matrixSize_; ++i) {
        delete [] matrixArray_[i];
    }
    delete [] matrixArray_;
}

void matrix::printMatrix() const
{
    for(int row = 0; row < matrixSize_; row++) {
        for(int col = 0; col < matrixSize_; col++) {
            cout << "test" << "  ";
            cout << matrixArray_[row][col] << endl;
        }// end col
        cout << endl;
    }//end row
}

1: Added '_' suffix and might as well make sure we don't accidentally get garbage here,

2: Added '_' suffix and I made private so it doesn't get tweaked from outside,

3: Added a 'size()' accessor member function,

4: Use initializer list to copy "matrixSize" parameter to "matrixSize_" member,

5: Removed the 'int**' and note we're using 'matrixArray_' - member,

6: The () after the new int[matrixSize_] tells the compiler to default initialize the columns for us (sets them to zero) In modern C++ this would be {} instead, but then I'm not sure if you're using a modern C++ compiler and both work.

7: Added a destructor to release the memory used

For similar reasons, I would encourage you to consider making your type names distinct: class Matrix or class matrix_t.

kfsone
  • 23,617
  • 2
  • 42
  • 74
  • Thanks for the point by point critique. I'm trying to learn best practices and this really helped. – PixelPusher Sep 21 '16 at 20:56
  • @TimBotelho Glad it helped! It's important to have some understanding of arrays and dynamic memory allocation, but modern C++ saves you a *lot* of the pain these things cause. Be sure to bookmark [vector](http://en.cppreference.com/w/cpp/container/vector), [unique_ptr](http://en.cppreference.com/w/cpp/memory/unique_ptr) and [shared_ptr](http://en.cppreference.com/w/cpp/memory/shared_ptr) for later reading - but not too late - save yourself a lot of pain :) – kfsone Sep 21 '16 at 21:01