Matrix slice has a couple problems:
- It is impossible define a variable with two possible types and have both active in the same scope.
- The return type of
int makes little sense. The matrix is sliced up, but then what? It can't be handed back to the caller to do anything with it.
This can be fixed with a union, but yikes! The bookkeeping on that will be a Smurfing nightmare. Don't do it!
The next is to always use a vector of vectors, but I don't like that idea for a couple reasons I'll get into below.
Instead I pitch a simple wrapper object around a single vector. This is done for two reasons:
- It preserves the ability to back a 1 dimensional matrix with a 1 dimensional container. If you have many rows of one column, all of the row data remains contiguous and cache friendly.
- It tends to be much faster. The data of one
vector is contiguous in memory and reaps the rewards of cache friendliness. A vector of vectors is basically a list of pointers to arrays of data, sending the poor CPU on an odyssey of pointer-chasing through memory to find the columns. If the columns are short, this can really, really hurt performance.
Here we go:
template<class TYPE>
class Matrix
{
private:
size_t mNrRows; // note size_t. This is unsigned because there is no reason
// for a matrix with a negative size. size_t is also guaranteed
// to fit anything you can throw at it.
size_t mNrColumns;
std::vector<TYPE> mVec;
public:
// make a default-initialized matrix
Matrix(size_t nrRows, size_t nrColumns) :
mNrRows(nrRows), mNrColumns(nrColumns), mVec(mNrRows * mNrColumns)
{
}
// make a def-initialized matrix
Matrix(size_t nrRows, size_t nrColumns, TYPE def) :
mNrRows(nrRows), mNrColumns(nrColumns), mVec(mNrRows * mNrColumns,
def)
{
}
// gimme a value and allow it to be changed
TYPE & operator()(size_t row, size_t column)
{
// could check for out of bounds and throw an exception here
return mVec[row * mNrColumns + column];
}
//gimme a value and do not allow it to be changed
TYPE operator()(size_t row, size_t column) const
{
return mVec[row * mNrColumns + column];
}
// gimme the number of rows
size_t getRows() const
{
return mNrRows;
}
// gimmie the number of columns.
size_t getColumns() const
{
return mNrColumns;
}
// printing convenience
friend std::ostream & operator<<(std::ostream & out, const Matrix & mat)
{
int count = 0;
for (TYPE val: mat.mVec)
{
out << val;
if (++count == mat.mNrColumns)
{
out << '\n';
count = 0;
}
else
{
out << ' ';
}
}
return out;
}
};
The vector member handles all of the heavy lifting so the Rule of Zero recommends leaving the copy and move constructors, assignment operators, and destructor up to the compiler.
What does this do to MatrixSlice? Well, first it now received and returns a Matrix instead of vector<vector> and int. The insides use Matrix and the confusion about 1D or 2D is just plain gone, resulting in a simpler function.
Matrix<int> MatrixSlice(const Matrix<int> & Input,
int Row1,
int Row2,
int Col1,
int Col2)
{
Matrix<int> OutPut(Row2-Row1 + 1,
Col2-Col1 + 1); // but what if Row1 > Row2?
int i2;
int j2= 0; // definitely need to initialize this sucker.
for (int i = Row1; i <= Row2; i++) // logical problem here: What if Row2 >= input.getRows()?
{
i2 = 0;
for (int j = Col1; j <= Col2; j++) // similar problem here
{
int DumyValue = Input(i, j);
OutPut(j2, i2) = DumyValue;
i2++;
}
j2++;
}
return OutPut;
}
Not that this completely ignores the very logical option of making slice a Matrix method. While it makes sense, it doesn't need to be a method and the stock recommendation is to prefer a free function. One good improvement is to make the function a template so that it can handle all sorts of Matrix in addition to Matrix<int>.
And finally, what happens to main?
int main()
{
//Define a matrix for test:
Matrix<int> Matrix2(4, 5, 1); // initialize matrix to all 1s
int R = 4;
int C = 4;
Matrix<int> MatrixInput(R, C); // default initialize the matrix
for (int i = 0; i < MatrixInput.getRows(); i++)
{
for (int j = 0; j < MatrixInput.getColumns(); j++)
{
int temp;
temp = i ^ 2 + j ^ 2;
// WARNING: ^ is XOR, not exponent. Maybe OP wants i XOR 2, but not
// likely. But if XOR is the desired operation, there is a lurking
// order of operation bug that needs to be addressed
MatrixInput(i, j) = temp;
}
}
std::cout << MatrixInput << '\n';
std::cout << MatrixSlice(MatrixInput, 0, 3, 1, 1);
return 0;
}
