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

}
Improve this question
9
  • You really should store the matrix in a one dimensional vector. Commented Sep 21, 2016 at 19:54
  • will somebody smack you if you use vector<vector<int> > instead? Commented Sep 21, 2016 at 19:54
  • @Jean-FrançoisFabre Only someone who cares a lot about cache locality. 1D vectors are better. Commented Sep 21, 2016 at 19:55
  • @vsoftco right about that. I did not see your comment :) I was talking about int ** versus vector<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. Commented Sep 21, 2016 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. Commented Sep 21, 2016 at 19:57

2 Answers 2

Reset to default
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! ;)

Improve this answer
Sign up to request clarification or add additional context in comments.

11 Comments

Actually OP just has to remove int**. Good catch!
Thanks @vsoftco, I updated my answer though to make it super-clear to everyone! ;)
and don't forget to create a copy constructor and an assignment operator that does not just copy the pointer value but duplicates the data, without memory leaks. Good luck, it's not that easy.
another trick: name your class members differently: m_matrix, _matrix to avoid mixups like this.
@Jean-FrançoisFabre You should really avoid names starting with a leading underscore. In certain cases using them is UB so it is just best to avoid the situation entirely.
|
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.

Improve this answer

2 Comments

Thanks for the point by point critique. I'm trying to learn best practices and this really helped.
@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, unique_ptr and shared_ptr for later reading - but not too late - save yourself a lot of pain :)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.