Practise Problem, Matrix class

[vidit_X]

Practise problem by ~s.o.s~
Q. Write a program which performs addition, subtraction, multiplication of matrices. The dimensions of both the matrices would be specified by the user (dynamic memory allocation required). Use of structure or a class to define the matrix would be a good idea. (Expert)

#include<iostream>
using namespace std;
class matrix
{
 int dim1,dim2;
 int **mat;
 public:
 matrix(int x=2,int y=2)
 {
  dim1=x;
  dim2=y;
  mat=new int*[dim1];
  for(int i=0;i<dim1;i++)
	  mat[i]=new int[dim2];
  for(int i=0;i<dim1;i++)
   for(int j=0;j<dim2;j++)
    mat[i][j]=0;
 }
 int returndim1()
 {
  return dim1;
 }
 int returndim2()
 { 
  return dim2;
 }
 void input()
 {
  cout<<"Enter the elements of matrix - ";
  for(int i=0;i<dim1;i++)
   for(int j=0;j<dim2;j++)
    cin>>mat[i][j];
 }
 void out()
 {
  for(int i=0;i<dim1;i++)
   {
    cout<<"\n";
    for(int j=0;j<dim2;j++)
	cout<<mat[i][j]<<" ";
   }
 } 
 matrix operator+(matrix x)
 {
  matrix c(dim1,dim2);
  if(dim1==x.returndim1() && dim2==x.returndim2())
   {    
    for(int i=0;i<dim1;i++)
      for(int j=0;j<dim2;j++)
	   c.mat[i][j]=this->mat[i][j]+x.mat[i][j];
	return c;
   }
  else
  {
   cout<<"Matrix cant be added";  
   return c;  
  }
 }
 matrix operator-(matrix x)
 {
  matrix c(dim1,dim2);
  if(dim1==x.returndim1() && dim2==x.returndim2())
   { 
    for(int i=0;i<dim1;i++)
      for(int j=0;j<dim2;j++)
	   c.mat[i][j]=this->mat[i][j]-x.mat[i][j];
	return c;
   }
  else
  {
   cout<<"Matrix cant be subtracted";  
   return c;  
  }
 }
 matrix operator*(matrix x)
 {
  matrix c(dim1,x.returndim2());
  if(dim2==x.returndim1())
   { 
    for(int i=0;i<dim1;i++)
      for(int j=0;j<x.returndim2();j++)
	     for(int k=0;k<dim2;k++)
	       c.mat[i][j]+=this->mat[i][k]*x.mat[k][j];
	return c;
   }
  else
  {
   cout<<"Matrix cant be multiplied";  
   return c;  
  }
 }
};
int main()
{
 int x,y;
 char ch;
 do{
 cout<<"Enter the dimension of matrix1 -";
 cin>>x>>y;
 matrix a(x,y);
 a.input();
 cout<<"\nEnter the dimension of matrix2 -";
 cin>>x>>y;
 matrix b(x,y);
 b.input();
 cout<<"\n1.Add\n2.Sub\n3.Multiply\n";
 cin>>x;
 if(x==1)
  (a+b).out();
 else if(x==2)
  (a-b).out();
 else if(x==3)
  (a*b).out();
 else
  cout<<"Wrong choice";
 cout<<"\nContinue(y/n)";
 cin>>ch;
 }while(ch=='y'||ch=='Y');
 cin.get();
 return 0;
}

Hows the coding? How the code can be more optimized?
Whats the complexity?Is it O(n3)?
Is there any better way for solving the above problem?
Any suggestions,comments or recommendations are welcomed.

[WaltP]

My initial reaction is it needs whitespace.
1) Indent 3-4 spaces, not 1
2) More spaces on a line for (int j = 0; j < dim2; j++) c.mat[i][j] += this->mat[i][k] * x.mat[k][j]; 3) Blank lines between logical sections

Makes the program more readable.

[vidit_X]

Thanks WaltP for your suggestion.
I'll take care of all that from next time. Any other suggestion?

[Nick Evan]

1. I wouldn't use "cout" statements in your operator-functions. If you want to use this class in a GUI-app for example, the cout statements would be useless. How about returning an empty matrix?

2. If you going to use this: mat[i]=new int[dim2]; be sure to delete[] the memory. This should be done in the destructor which is missing..

3.

if(x==1)
            (a+b).out();
        else if(x==2)
            (a-b).out();
        else if(x==3)
            (a*b).out();
        else ...

switch case?

Just to name a few things

[vidit_X]

@niek_e - Thanks for your suggestions.
1. I agree, my operator class should not have cout. As an operator never outputs, it just performs operation. Got your point.

2. Oops, I missed it. It surely needs freeing, here's the destructor code -

~matrix()
{
 for(int i=0;i<dim2;i++)
  delete[] mat[i];
 delete[] mat;
}

3. I intentionally didn't used switch bcoz there were only 3 cases, I thought if else ladder would be better.

[mrnutty]

I have commented on your code with code comments , see below :

#include<iostream>
using namespace std;
// A TEMPLATE CLASS WILL BE BETTER
class matrix //USUALLY A CLASS NAME SHOULD START WITH A CAPITAL LETTER( from convention)
{
 //is the user able to change these ? if not then make it constant
 int dim1,dim2; // a better name would be Row and Column
 int **mat;
 public:
 matrix(int x=2,int y=2) //No need for default ctor of that form. Either Matrix() or Matrix(const int  
                                     //MaxRow, const int MAXCOL)
 {
  dim1=x;
  dim2=y;
 //make a private function that does this, which will make you code neater
  mat=new int*[dim1];
  for(int i=0;i<dim1;i++)
	  mat[i]=new int[dim2];
  for(int i=0;i<dim1;i++)
   for(int j=0;j<dim2;j++)
    mat[i][j]=0;
 }
 int returndim1() //whats dim1 the Row or the column ?
 {
  return dim1;
 }
 int returndim2() //goto : returndim1 comment
 { 
  return dim2;
 }
 //why ?Just let the user handle this
 void input()
 {
  cout<<"Enter the elements of matrix - ";
  for(int i=0;i<dim1;i++)
   for(int j=0;j<dim2;j++)
    cin>>mat[i][j];
 }
 //out is a bad name, and what if user wan't a specific format ?
 void out()
 {
  for(int i=0;i<dim1;i++)
   {
    cout<<"\n";
    for(int j=0;j<dim2;j++)
	cout<<mat[i][j]<<" ";
   }
 } 
 //use const matrix x, also make this function const-corrected( look it up)
 matrix operator+(matrix x)
 {
  matrix c(dim1,dim2);
  if(dim1==x.returndim1() && dim2==x.returndim2())
   {    
    for(int i=0;i<dim1;i++)
      for(int j=0;j<dim2;j++)
	   c.mat[i][j]=this->mat[i][j]+x.mat[i][j];
	return c;
   }
  else
  {
   cout<<"Matrix cant be added";  
   return c;  
  }
 }
//goto comment on operator+
 matrix operator-(matrix x)
 {
  matrix c(dim1,dim2);
  if(dim1==x.returndim1() && dim2==x.returndim2())
   { 
    for(int i=0;i<dim1;i++)
      for(int j=0;j<dim2;j++)
	   c.mat[i][j]=this->mat[i][j]-x.mat[i][j];
	return c;
   }
  else
  {
   cout<<"Matrix cant be subtracted";  
   return c;  
  }
 }
//goto comment on operator+
 matrix operator*(matrix x)
 {
  matrix c(dim1,x.returndim2());
  if(dim2==x.returndim1())
   { 
    for(int i=0;i<dim1;i++)
      for(int j=0;j<x.returndim2();j++)
	     for(int k=0;k<dim2;k++)
	       c.mat[i][j]+=this->mat[i][k]*x.mat[k][j];
	return c;
   }
  else
  {
   cout<<"Matrix cant be multiplied";  
   return c;  
  }
 }
};
int main()
{
 int x,y;
 char ch;
 do{
 cout<<"Enter the dimension of matrix1 -";
 cin>>x>>y;
 matrix a(x,y);
 a.input();
 cout<<"\nEnter the dimension of matrix2 -";
 cin>>x>>y;
 matrix b(x,y);
 b.input();
 cout<<"\n1.Add\n2.Sub\n3.Multiply\n";
 cin>>x;
 if(x==1)
  (a+b).out();
 else if(x==2)
  (a-b).out();
 else if(x==3)
  (a*b).out();
 else
  cout<<"Wrong choice";
 cout<<"\nContinue(y/n)";
 cin>>ch;
 }while(ch=='y'||ch=='Y');
 cin.get();
 return 0;
}

Overall, I would give you a --C. Here are the main suggestions :

1) Clean your code, make private function that helps your code. You
will know when you need a private helper function, when you see a block of code that can be represented as a function and it makes sense to do so.

2) Make your class const-corrected.
3) Use const as much as you can.
4) Rename your class, use convention naming.

5) Also notice that your operator+, operator-,operator* code is redundant, you use almost every same code inside of each of those function. Before I suggest something, can you think of how you can simplify this ? Hint : your operator+ code could look like this :

Matrix operator+(const Matrix& rhs)const{
 return _arithmeticHelper(rhs,ADD);
}

Fix those and post back.

[vidit_X]

@firstPerson - that's a whole lot of corrections. Thanks for all that :)
I'll revamp all my code and post it here in my next post.

Meanwhile, I have some doubts regarding those comments and corrections.
1. You emphasized on using const wherever I can, I'll but may I know why it is good?
2. If I use parameterless constructor like as u said Matrix(), how'll I initialize dimensions?
3. Can you give me a little more hint on that private function, to be used in constructor?
4. const-corrected means using const parameteres in functions?
5. Why pass matrix in operator function as reference when I wont be modifying them?

Any other comments or corrections are welcomed.

[mrnutty]

>>1. You emphasized on using const wherever I can, I'll but may I know why it is good?

Because making things const not only enables the compiler
to maybe optimize things, it also promises that the variable
wont be altered, and in general, if its not needed to be altered, then make it const.

>>2. If I use parameterless constructor like as u said Matrix(), how'll I initialize dimensions? I am not saying only have Matrix() constructor, have multiple overloaded constructor.

Matrix a ; //default constructor Matrix()
Matrix b = Matrix(3,3); // Matrix(const int& Row, const int& Col);
Matrix c = Matrix(3,3 0 ); //Matrix(const int& Row, const int& Col, const int& initValue);
a = Matrix(2,4); // init Matrix a.

>>3. Can you give me a little more hint on that private function, to be used in constructor?
Make a private function like the following pseudo function:

void _arithmeticHelper(const Matrix& other, const int& Operator){
 for i = 0 to Row
   for j = 0 to Col
    switch( Operator){
     case ADD : _myMatrix[i][j] + other._myMatrix[i][j]; break;
     case SUBTRACT : ... break;
     case MULTIPLY : ... break;
    }
}

Where ADD, SUBTRACT, and MULTIPLY is some private static const,
maybe declared like so :

class Matrix{
//private helpers
private: 
  static const int ADD = '+';
  static const int SUBTRACT = '-';
  static const int MULTIPLY = '*';
}

>>4. const-corrected means using const parameteres in functions?
There is more to it, look at this function for example :

void print(const Matrix& mat) const {
 ...
}

Notice the const's, the first one says that it will not modify the Matrix mat, the second one( the far right) says that that print function, which is inside a class Matrix, will not change any member variables.

>>5. Why pass matrix in operator function as reference when I wont be modifying them?

You are correct, in that using a reference parameter means that a function can change the parameter, but using a reference parameter also means that no copies will be made and that the
parameter will "point to" or "refer to" the argument passed, and thus the compiler will make no copies. So using reference parameters has 2 main properties, (1)no copies will be made of the
passed argument, and (2)the variable that is referring to the passed argument can be changed. If property 2 is not what you want then you put a const before it like so ,

void foo(const Matrix& referingVariable);

That not only saves copying, but it also promises that referingVariable wont be changed. The above is faster than below
since no copying is done :

//below
void foo(Matrix mat); //mat is a copy of the Matrix passed

[dusktreader]

Here is a working example with a few notes at the bottom:

#include <iostream>

using namespace std;

enum ops{ ADD, SUB, MLT };

template <class T>
class Matrix{
private:
    T* mat;
    int rows;
    int cols;
    int sz;

    void initMat( int rows, int cols ){
        if( rows <= 0 || cols <= 0 ){
            mat = NULL;
            return;
        }
        this->rows = rows;
        this->cols = cols;
        sz = rows * cols;
        mat = new T[sz];
    }


public:
    Matrix(){
        mat = NULL;
    }

    Matrix( int rows, int cols ){
        initMat( rows, cols );
    }

    Matrix( int rows, int cols, T initVal ){
        initMat( rows, cols );
        for( int i=0; i<sz; i++ )
            mat[i] = initVal;
    }

    Matrix( const Matrix& other ){
        initMat( other.rows, other.cols );
        memcpy( mat, other.mat, sz * sizeof(T) );
    }

    ~Matrix(){
        delete [] mat;
    }

    T& operator()( int row, int col ) const{
        return mat[ cols * row + col ];
    }

    Matrix operator+( const Matrix& other ) const{
        return arithmeticOperator( other, ADD );
    }

    Matrix operator-( const Matrix& other ) const{
        return arithmeticOperator( other, SUB );
    }

    Matrix operator*( const Matrix& other ) const{
        return arithmeticOperator( other, MLT );
    }

    Matrix arithmeticOperator( const Matrix& other, ops op ) const{
        if( mat == NULL || other.mat == NULL || other.rows != rows || other.cols != cols )
            return Matrix();
        Matrix neo( rows, cols );
        switch( op ){
            case ADD: for( int i=0; i<sz; i++ ) neo.mat[i] =  mat[i] + other.mat[i]; break;
            case SUB: for( int i=0; i<sz; i++ ) neo.mat[i] =  mat[i] - other.mat[i]; break;
            case MLT: for( int i=0; i<sz; i++ ) neo.mat[i] =  mat[i] * other.mat[i]; break;
        }
        return neo;
    }

    friend ostream& operator<<( ostream& out, const Matrix& self ){
        if( self.mat == NULL ){
            out << "<Invalid Matrix>" << endl;
            return out;
        }
        for( int row=0, idx=0; row<self.rows; row++ ){
            out << "|   ";
            for( int col=0; col<self.cols; col++ )
                out << self.mat[idx++] << "   ";
            out << "|" << endl;
        }
        return out;
    }

};

int main(int argc, char *argv[]){
    Matrix<int> A( 10, 10, 10 );
    A( 5, 5 ) = 80;
    Matrix<int> B = A;
    B( 5, 5 ) += 19;
    cout << A << endl << endl;
    cout << B << endl;
    Matrix<double> C( 5, 5, 3.14 );
    C( 1, 1 ) = 1.5;
    Matrix<double> D = C * C;
    cout << D << endl;
    Matrix<char> E;
    cout << E << endl;
    Matrix<char> F( 0, 2 );
    cout << F << endl;
}

Notes:
1. I prefer to keep my ND arrays saved in a 1D array, and then I use modular arithmetic to manage indexing. For 2D arrays this is very simple:

idx = r * cols + c;
r = idx / cols;
c = idx % cols

Although I usually use a specialized divmod() function that does both division and modulus at the same time for efficiency ( no double divide or slow % operator ).

2. There is a for loop in each case of the switch statement in arithmeticOperator(). This does not look as aesthetically pleasing, but it keeps the code from executing a switch on every single iteration of the loop.

3. If you're going to be using output streams to print your class, you might as well overload the ostream (<<) operator.

You could expand this class to have a great deal of more functionality with some additions:
1. overload the arithmetic-assignment operators ( +=, -=, *= )
2. overload scalar arithmetic operators such that the operation is done to each element of the matrix.
3. Implement reshaping operations. (i.e. change rows and cols correctly)
4. Create slicing operations so that a row, a column, or a sub-matrix can be extracted

[vidit_X]

@firstPerson - Thanks again for answering my queries :)
@dusktreader - Thanks for the code, I'll take hints from that. I thought of using single dimensional array for representing the matrix but then change to two dimensional to support user's view of the matrix. I might implement additional functionalities once I recode all the above functionalities properly. Also, I didn't got what you meant by this "but it keeps the code from executing a switch on every single iteration of the loop" :-/

I have almost completed the recoded code. But in the meanwhile, have some more questions which I got while coding acc. to the comments.
1. Starting the name of functions with underscore is another convention like _arithmetichelper? I have seen many.
2. We group the operators(+,-,*) functionality together in a different functions bcoz they are almost simliar, right?
Also for the code in constructor, we group them in a function bcoz of similar functionality, right?
3. Why to use static const int variables like ADD, SUB etc to be passed as parameters to _arithmetichelper()? I mean cant we use any other parameter like const ints and pass 0 or 1 as argument?
4. What is the benefit of making _arthimetichelper() as private?

Any more comments or corrections are welcomed.

[vidit_X]

Here's the revamped code ...

#include<iostream>
#include<iomanip>

using namespace std;

template <class X=int> class Matrix 
{
 private:
 static const int ADD='+';
 static const int SUB='-';
 static const int MLT='*';
 int row,col;
 X **mat;

 int init(const int&x,const int&y) {
  row=x;
  col=y;
  mat=new X*[row];
  for(int i=0;i<row;i++)
	  mat[i]=new X[col];
  return 0;  
 }

 int initv(const X& init) {
  for(int i=0;i<row;i++)
   for(int j=0;j<col;j++)
    mat[i][j]=init;
  return 0;
 }

 int initv(const Matrix& init) {
  for(int i=0;i<row;i++)
   for(int j=0;j<col;j++)
    mat[i][j]=init(i,j);
  return 0;
 }

 Matrix _arthimetichelper(const Matrix &other,const int& op)const {
  if(mat==NULL||other.mat==NULL)
   return Matrix();
  else
  switch(op)
  {
   case ADD:
    if(row!=other.returnrow()||col!=other.returncol())
     return Matrix();
    else
    {
     Matrix r(row,col);
     for(int i=0;i<row;i++)  
	  for(int j=0;j<col;j++)
	  r(i,j)=mat[i][j]+other(i,j);
	 return r;
    }
    break;
   case SUB:
    if(row!=other.returnrow()||col!=other.returncol())
     return Matrix();
    else
    {
     Matrix r(row,col);
     for(int i=0;i<row;i++)  
	  for(int j=0;j<col;j++)
	   r(i,j)=mat[i][j]-other(i,j);
	 return r;  
    }
    break;
   case MLT:
    if(col!=other.returnrow())
     return Matrix();
    else
    {
     Matrix r(row,other.returncol());
     for(int i=0;i<row;i++)  
	  for(int j=0;j<other.returncol();j++)
	   for(int k=0;k<col;k++)
	    r(i,j)+=mat[i][k]*other(k,j);
	 return r;  
    }
   break;
  };
 }
  
 public:
 Matrix() {
  mat=NULL;
 }
 
 Matrix(const int& x, const int& y) {
  init(x,y);
  initv(0);
 }
 
 Matrix(const int&x, const int& y, const X& init) {
  init(x,y);
  initv(init);
 }
 
 Matrix(const Matrix &other) {
  init(other.returnrow(),other.returncol());
  initv(other);
 }
 
 ~Matrix() {
  for(int i=0;i<row;i++)
    delete[] mat[i];
  delete[] mat;
 }
  
 int returnrow() const {
  return row;
 }
 
 int returncol() const { 
  return col;
 }
 
 X& operator()(const int& x,const int& y) const {
  if(x>=row||x<0||y>=col||y<0)
   cout<<"Error! Enter correct dimensions";
  else
   return mat[x][y];
 }
 
 friend ostream& operator<<(ostream& stream,const Matrix& y) {
  if(!y.mat) {
   stream<<"Empty Matrix\n";
   return stream;
  }
  else {
  for(int i=0;i<y.row;i++) {
    stream<<"\n";
    for(int j=0;j<y.col;j++)
	stream<<setw(3)<<y(i,j)<<" ";
   }
  return stream;
  }
 } 
 
 Matrix operator+(const Matrix& y) const{
  return _arthimetichelper(y,ADD);
 }
 Matrix operator-(const Matrix& y) const{
  return _arthimetichelper(y,SUB);
 }
 Matrix operator*(const Matrix& y) const{
  return _arthimetichelper(y,MLT);
 }
};

int main() {
 int i,j,x,y;
 int ch;
 do{
 cout<<"Enter the dimension of Matrix1 -";
 cin>>x>>y;
 Matrix<> a(x,y);
 cout<<"Enter the elements of Matrix1 -";
 for(i=0;i<x;i++)
  for(j=0;j<y;j++)
   cin>>a(i,j);
 cout<<"\nEnter the dimension of Matrix2 -";
 cin>>x>>y;
 Matrix<> b(x,y);
 cout<<"Enter the elements of Matrix2 -";
 for(i=0;i<x;i++)
  for(j=0;j<y;j++)
   cin>>b(i,j);
 cout<<"\n1.Add\n2.Sub\n3.Multiply\n";
 cin>>x;
 if(x==1)
  cout<<(a+b);
 else if(x==2)
  cout<<(a-b);
 else if(x==3)
  cout<<(a*b);
 else
  cout<<"Wrong choice";
 cout<<"Continue(y/n)";
 cin>>ch;
 }while(ch=='y'||ch=='Y');
 cin.get();
 return 0;
}

Any other comments or suggestions? :)

[dusktreader]

@dusktreader - Thanks for the code, I'll take hints from that. I thought of using single dimensional array for representing the matrix but then change to two dimensional to support user's view of the matrix.

notice that I overloaded the () operator to allow access to an item by (row, col), so Mat( i, j ) will return the item at the ith row, jth column. You can easily expose 2d functionality ( or higher D if you are careful and clever ) to a user while preserving the underlying 1D structure.

Also, I didn't got what you meant by this "but it keeps the code from executing a switch on every single iteration of the loop" :-/

@first person suggested a helper function that looked like this:

void _arithmeticHelper(const Matrix& other, const int& Operator){
 for i = 0 to Row
   for j = 0 to Col
    switch( Operator){
     case ADD : _myMatrix[i][j] + other._myMatrix[i][j]; break;
     case SUBTRACT : ... break;
     case MULTIPLY : ... break;
    }
}

Notice that in this code, the switch statement is inside of the nested for loop. This means that the program will be required to branch ( execute an if statement ) in every single iteration of the loop.

I suggest this as an alternative:

Matrix arithmeticOperator( const Matrix& other, ops op ) const{
    if( mat == NULL || other.mat == NULL || other.rows != rows || other.cols != cols )
        return Matrix();
    Matrix neo( rows, cols );
    switch( op ){
        case ADD: for( int i=0; i<sz; i++ ) neo.mat[i] =  mat[i] + other.mat[i]; break;
        case SUB: for( int i=0; i<sz; i++ ) neo.mat[i] =  mat[i] - other.mat[i]; break;
        case MLT: for( int i=0; i<sz; i++ ) neo.mat[i] =  mat[i] * other.mat[i]; break;
    }
    return neo;
}

Notice that here, there is a for loop in each case of the switch statement, and the switch is not nested in any local loop. While it may lose aesthetics for specifying the loop structure three times, there is no branching occuring at every iteration. Indeed, it would make just as much sense to put the arithmetic loops in the operator funcitons themselves instead of specifying a helper function here. I just used this method to show how @firstPerson's idea could work in compilable code

1. Starting the name of functions with underscore is another convention like _arithmetichelper? I have seen many.

This is just a naming convention that people often use for private variables, private (auxillery) functions. It has no functional meaning. It is simply a convention

2. We group the operators(+,-,*) functionality together in a different functions bcoz they are almost simliar, right?

That would be the motivation. Since each is essentially running the same loop, we could group them to all use the same loop. However, there is the branching problem I mentioned earlier that pretty much eliminates the benefit. Generally, you want to avoid duplicate (or very similar) code. However, I don't see a good way of aliasing the arithmetic operators. Perhaps instead of passing an ENUM to the helper function, you could pass a function pointer to the arithmetic operator itself? In this case, this seems like a trivial correctness / optimization problem.

Also for the code in constructor, we group them in a function bcoz of similar functionality, right?

That is correct. Instead of having 3 different constructors all containing identical code, i grouped that functionality into 1 function. Notice that this is a private function. This ensures that only internal functions of the Matrix class can call this code. It will keep client code from invalidating the matrix

3. Why to use static const int variables like ADD, SUB etc to be passed as parameters to _arithmetichelper()? I mean cant we use any other parameter like const ints and pass 0 or 1 as argument?

To me, this is a preference only. For the native types (int, float, double, etc...) I never pass by reference unless the function should update the value. A pointer or reference is at least 32 bits. In the worst case scenario, a 64 bit type, passing by reference will save you only 32 bits (4 Bytes). So, with the native types, I never pass const references. If you are hellbent on keeping the function from changing (intenally only) the value of a function argument, you can use const. However, if you aren't passing by reference, any changes to variable will only be visible within the function, so I rarely worry about it. When it comes to any other types, classes, structs, etc, you should use the reference variable. For instance, if you needed to pass a vector<double> with 100 items, you wouldn't want the program to have to copy all that data into a local (to the function) variable. Instead, you could pass a const reference, the function would only have to copy 32 bits, and you would be guranteed that the function could not change the values in the vector. My conclusion: for native types, passing by value is fine, but for containers, classes, and structs use const references.

4. What is the benefit of making _arthimetichelper() as private?

You wouldn't want any outside code to call this function, because the function only makes logical sense within the scope of the class. For instance, ADD is not defined outside of the class, so how would the client code know what to pass as the op argument? Furthermore, client code can simply use the operators, so why expose the auxiliary function?

[dusktreader]

Any other comments or suggestions? :)

Line 24 (and others): Watch out for shadowing class members with function arguments. There is a function called init, so you should probably make this argument initVal. This is more readable anywya.

Line 34 (and others): Inside of the scope of the Matrix class, you have access to the mat array member. So, you should use direct access instead of accessor functions within this scope. So use mat[j] = other.mat[j]. This is also more readable, and it is more obvious what is going on.

Line 45 (and others): Move this conditional above the switch. There is no use repeating the same code in all the branches.

Line 49 (and others): Move the initialization above the switch. No use repeating.

Line 53 (and others): Move the return statement below the switch. No use repeating.

Line 69: Why is the conditional for the MLT case different? For a per-element multiplication, the constraints should be the same as the other operations. Also, just use other.row since it is within scope. Cohesion and consistency are as important as readability.

Line 134: I'm surprised your compiler signed off on this. The setw function isn't templated ( on my compiler ), so I couldn't include this. Also, would the setw know what to do if the X was a user defined type. If you had a type that was overloaded for +, -, *, and << you could create a Matrix of this type. However, setw wouldn't know what to do.

Overall, though, the code is much better. But always make sure that you understand all of the semantics behind your syntax before you use it!

[vidit_X]

Thanks dusktreader for answering :)

notice that I overloaded the () operator to allow access to an item by (row, col), so Mat( i, j ) will return the item at the ith row, jth column. You can easily expose 2d functionality ( or higher D if you are careful and clever ) to a user while preserving the underlying 1D structure.

I too overloaded (), but still if we choose two dimensional array, then it'll have similar memory allocation as user might expect for a matrix. Anyway, I think we both are right and it can be implemented either way.

Actually, I looked at your code while reading the line "it keeps the code from executing a switch on every single iteration of the loop" and therefore got confused.

My conclusion: for native types, passing by value is fine, but for containers, classes, and structs use const references.

Nice explanation. Got your point.

Line 34 (and others): Inside of the scope of the Matrix class, you have access to the mat array member. So, you should use direct access instead of accessor functions within this scope. So use mat[j] = other.mat[j]. This is also more readable, and it is more obvious what is going on.

Mat[][] is a private member of class Matrix object other, can it be directly accessed? I don't think so.

Line 45 (and others): Move this conditional above the switch. There is no use repeating the same code in all the branches.

It is same for addition and subtraction, not for multiplication.

Line 49 (and others): Move the initialization above the switch. No use repeating.

The Matrix is similar in case of addition & subtraction but not in case of multiplication and therefore.

Line 53 (and others): Move the return statement below the switch. No use repeating.

Since, Matrix will be local as due to above reason, moving return out of the switch won't work.

Line 69: Why is the conditional for the MLT case different? For a per-element multiplication, the constraints should be the same as the other operations. Also, just use other.row since it is within scope. Cohesion and consistency are as important as readability.

Matrix multiplication is not per element multiplication, we can even multiply two matrices of 3 X 4 & 4 X 5 dimensions, only the column of first matrix should be equal to row of second matrix. And how can I access other.row directly, row is a private member.

Line 134: I'm surprised your compiler signed off on this. The setw function isn't templated ( on my compiler ), so I couldn't include this. Also, would the setw know what to do if the X was a user defined type. If you had a type that was overloaded for +, -, *, and << you could create a Matrix of this type. However, setw wouldn't know what to do.

I didn't got it. Is it like, as X can be any type(user-defined), setw() wont know how to work with it?

Overall, though, the code is much better. But always make sure that you understand all of the semantics behind your syntax before you use it!

I code only after I understand. I don't like copying, just for showing ;)

Thanks again buddy for your help. Any more suggestions? :)

[dusktreader]

Thanks dusktreader for answering :)
Actually, I looked at your code while reading the line "it keeps the code from executing a switch on every single iteration of the loop" and therefore got confused.

The switch statement is really just a convenience wrapper for a regular conditional (if). A for loop already has a conditional that is executed in every iteration. Usually this is a check to ensure that the index hasn't exceeded the limit of the loop: for( int i=0; i<n; i++ ){...} If you can help it, you don't want there to be another conditional inside of the for loop, otherwise, the program has to check it on every iteration.
So, this:

for( ){
  if( C == A )
      someOperation();
  if( C == B )
      someOtherOperation();
}

Has to check the value of C on every iteration of the loop. Though it doesn't look as clean, it would be more efficient to write:

if( C == A )
    for()
        someOperation();
if( C == B )
    for()
        someOtherOperation();

This way, the test on the value of C is only performed once. This is a small optimization, but in a large project, simple boosts can gain a lot!

Mat[][] is a private member of class Matrix object other, can it be directly accessed? I don't think so.

Actually, it can directly access the private members of another class of the same type. Try this code out:

#include <iostream>
using namespace std;
class Dummy{
private:
    int _a;
public:
    Dummy(){
        _a = 0;
    }

    Dummy( int a ){
        _a = a;
    }

    Dummy( const Dummy& other ){
        _a = other._a;
    }

    int a() const{
        return _a;
    }
};

int main(int argc, char *argv[]){
    Dummy d(4);
    Dummy e = d;
    cout << e.a() << endl;
    return 0;
}

I use this property to my advantage all the time, as it can make the code neater ( you don't have to use accessor functions).

It is same for addition and subtraction, not for multiplication..............Matrix multiplication is not per element multiplication, we can even multiply two matrices of 3 X 4 & 4 X 5 dimensions, only the column of first matrix should be equal to row of second matrix. And how can I access other.row directly, row is a private member.

There are 4 types of multiplication for Matrices, and you need to be especially careful that you class clearly defines the difference.
1. Scalar Multiplication: Each element of the matrix is multiplied by the same scalar value ( int, float, etct )
Matrix A * int i
2. Element-wise Muliplication: Each element of one matrix is multiplied by the corresponding element of another matrix. This is not very useful, but can occasionally be used.
Matrix A[j] * Matrix B[j]
3. Dot Product Multiplication: Each [i,j] element of the product matrix equals the dot product of the ith row of A and the jth column of B
4. Cross Product: The most complicated. Pretty hard to describe in brief.

With matrix ( or vector )type classes, I always define an explicit crossProduct() and dotProduct() function. I never overload the * operator for these because their semantic meaning is so much different. I want the user to carefully choose between the two, and I want no assumptions made about which the * operator will implement. It may be a good idea to leave the * operator out, since element-wise multiplication is rarely useful. Also, since the dot product operation does not function like the element-wise arithmetic operations, you would not want to put this functionality in the arithmetic helper function. Also, see my above explanation of accessing private members from a class instance of the same type.

Since, Matrix will be local as due to above reason, moving return out of the switch won't work........

First, you should move the dot product out of the helper function. See above.
Without this different functionality mixed in the homogenous arithmetic operations, the tests ( A.mat != null, etc ) and the initialization could and should be moved out of the switch statement.

I didn't got it. Is it like, as X can be any type(user-defined), setw() wont know how to work with it?

Exactly. X can be any type, as long as all the correct operators ( +, -, *, << ) are defined in that type. So, if you wanted to implement, say, a ComplexNumber class, you could make a matrix of these as long as all of the operators, etc were defined correctly in the ComplexNumber class. However, setw is not a templated function, so it has no idea how to adjust the width for any type it may encounter. As far as I know, setw only works correctly with standard numeric types. I'm not completely sure of the details of this problem, but my compiler complained when I tried to include a setw. That's too bad, because I'm a stickler for nicely formatted output! :S

Thanks again buddy for your help. Any more suggestions? :)

That's all I have for now!

[vidit_X]

Thanks to all, for their valuable suggestions and comments :)

Thread Solved.