Addition of Matrix in C++ using OOP

Question

the following code is not giving the correct output. Matrix data is displaying perfectly, but after the addition of two objects M1 and M2, it did not display the correct output. If I use setData to input data in the matrix, data is stored perfectly, but the addition is not performing correctly. kindly suggest to me how can I correct this logical error?

#include <iostream>
#include <string.h>
using namespace std;

class Matrix{
    private:
        void Allocate();
        int noOfRows;
        int noOfColumns;
        int **data;
    public:
        Matrix(int noOfRows, int noOfColumns);
        void setData();
        void displayData();
        ~Matrix();
        Matrix (const Matrix &ref);
        Matrix operator + (const Matrix &m);
        void operator = (const Matrix &M );
        Matrix& operator = (int x);
};

Matrix::Matrix(int inr=0, int inc=0){
    noOfRows=inr; noOfColumns=inc;
    Allocate();
}

Matrix::Matrix (const Matrix &ref){
    Allocate();
    for(int r=0;r<ref.noOfRows;r++)
        for(int c=0;c<ref.noOfColumns;c++)
            data[r][c]=ref.data[r][c];
}

void Matrix :: operator = (const Matrix &M ) { 
    Allocate();
        noOfRows = M.noOfRows;
        noOfColumns = M.noOfColumns;
        data=M.data;
      }

Matrix& Matrix :: operator = (int x){
    Allocate();
    for(int r=0;r<noOfRows;r++)
        for(int c=0;c<noOfColumns;c++)
            data[r][c]=x;
        return *this;
      }

void Matrix::Allocate(){
    data=new int*[noOfRows];
    for(int i=0;i<noOfRows;i++)
        data[i]=new int[noOfColumns]();
}

void Matrix::setData(){
    for(int r=0;r<noOfRows;r++){
        for(int c=0;c<noOfColumns;c++){
            cout<<"Enter ...";cin>>data[r][c];
        }
    cout<<endl; 
    }
}

Matrix Matrix::operator + (const Matrix &m){
    Matrix ms(m.noOfRows,m.noOfColumns);
    for (int i=0; i<m.noOfRows; i++) 
        for (int j=0; j<m.noOfColumns; j++)
        ms.data[i][j] = data[i][j]+m.data[i][j];
    
    return ms;
    }

void Matrix::displayData(){
    for(int r=0;r<noOfRows;r++){
        for(int c=0;c<noOfColumns;c++)
            cout<<data[r][c]<<"\t";
        cout<<endl;
    }
}

Matrix::~Matrix(){
    for (int i = 0; i < noOfRows; ++i)
        delete[] data[i];
    delete [] data;
}

int main(){
    Matrix M3(2,2);M3=0;
    Matrix M1(2,2);M1=1;
    Matrix M2(2,2);M2=2;
//M1.setData();M2.setData();M3.setData();   
    cout<<"\n Matrix A = "<<endl;
    M1.displayData();
    cout<<"\n Matrix B = "<<endl;
    M2.displayData();
    cout<<"\n Matrix C = "<<endl;
    M3 = M1;
    M3.displayData();
    cout<<"\n Sum of Matrix = "<<endl;
    M3 = M1 + M2;
    M3.displayData();

    return 0;
}

Output is here for detail

Please also note that a copy assignment operator usually returns a reference to the class, not `void` — Bob__, Mar 29 '22 at 20:51

score 0 · Accepted Answer · answered Mar 29 '22 at 21:57

Copy constructor and assignment operator were both broken. Let's take a quick stroll to see what went wrong

Matrix::Matrix (const Matrix &ref){
    // noOfRows and noOfColumns have not been initialized. Their values are unknown so 
    // the rest of this function is a crapshoot. Could do anything.
    Allocate();
    for(int r=0;r<ref.noOfRows;r++)
        for(int c=0;c<ref.noOfColumns;c++)
            data[r][c]=ref.data[r][c];
}

// An assignment operator is expected to return Matrix &
void Matrix :: operator = (const Matrix &M ) { 
    // noOfRows and noOfColumns have not been updated. Could be wrong, resulting in 
    // allocate allocating the wrong amount of storage. Moot point since this 
    // allocation is never used. See below.
    Allocate();
        noOfRows = M.noOfRows;
        noOfColumns = M.noOfColumns;
        data=M.data; // both instances share M's data. This will fail sooner or later
                     // If the program lives long enough, both instances will go out
                     // of scope and double delete the allocation.
                     // Also leaks allocation pointed at by `data`.
      }

I was in a hurry so I didn't put much thought into fixing the assignment operator and just used the Copy and Swap Idiom. It's very possible that there is a more efficient solution, but copy and swap is usually fast enough and almost impossible to get wrong. Makes a great starting point and benchmarking will tell you if it's a problem.

Matrix::Matrix(const Matrix &ref):
        noOfRows(ref.noOfRows),
        noOfColumns(ref.noOfColumns) // initialize row and columns
{
    Allocate(); // now safe to use
    for (int r = 0; r < ref.noOfRows; r++)
        for (int c = 0; c < ref.noOfColumns; c++)
            data[r][c] = ref.data[r][c];
}

Matrix& Matrix::operator =(Matrix M) // correct return type, and does the heavy
                                     // lifting with the copy constructor
{
    std::swap(noOfRows, M.noOfRows);
    std::swap(noOfColumns, M.noOfColumns);
    std::swap(data, M.data);
    return *this;
}

Addition of Matrix in C++ using OOP

1 Answers1