I have a matrix class with fields like this:

template <typename T>
class Matrix
{
private:
    T **matrix = nullptr;
    int rows; 
    int cols; 

At this stage, I have written an assignment operator and a copy constructor. But firstly, there is code duplication, how can it be avoided, and secondly, they seem very similar to me, how can these methods be improved to look normal?

Matrix(const Matrix &matrix_) : rows(matrix_.rows), cols(matrix_.cols)
        {
            matrix = static_cast<T **>(new T *[rows]);

            for (int i = 0; i < rows; i  )
            {
                matrix[i] = static_cast<T *>(new T[cols]);
            }
            for (int i = 0; i < rows; i  )
            {
                for (int j = 0; j < cols; j  )
                {
                    matrix[i][j] = matrix_[i][j];
                }
            }
        }

        Matrix &operator=(const Matrix &matrix_)
        {
            if (&matrix == this)
            {
                return *this;
            }
            clean();

            rows = matrix_.rows;
            cols = matrix_.cols;

            matrix = static_cast<T **>(new T *[rows]);

            for (int i = 0; i < rows; i  )
            {
                matrix[i] = static_cast<T *>(new T[cols]);
            }
            for (int i = 0; i < rows; i  )
            {
                for (int j = 0; j < cols; j  )
                {
                    matrix[i][j] = matrix_[i][j];
                }
            }
        }

        void clean()
        {
            if (cols > 0)
            {
                for (int i = 0; i < rows; i  )
                {
                    delete[] matrix[i];
                }
            }
            if (rows > 0)
            {
                delete[] matrix;
            }
        }

According to the condition of the assignment, it is forbidden to use STL containers, I must implement the controls myself

CodePudding user response：

Use a 1D array then. It will be much cleaner and simpler and faster than an array of pointers to arrays...

template <typename T>
class Matrix
{
private:
    T* data; // Flattened matrix with size = rows*cols. Be careful,
             // for really big matrices this multiplication could overflow!!!

    // You can use unsigned type since negative size is a nonsense...
    unsigned int rows;
    unsigned int cols;

public:
    Matrix(const Matrix& other) 
        : data(new T[other.rows*other.cols])
        , rows(other.rows)
        , cols(other.cols)
    {
        /// You can do this:
        /// for(int i = 0; i < rows*cols;   i)
        ///    data[i] = other.data[i];
        /// or simply call the copy assign operator:
        operator=(other);
    }

    Matrix& operator=(const Matrix& other)
    {
        assert(rows*cols == other.rows*other.cols);
        for(int i = 0; i < rows*cols;   i)
            data[i] = other.data[i];
        return *this;
    }
};

Note: You will also need to define the constructor(s) and destructor of course... I'm pretty sure you can't make it much simpler than this...

CodePudding user response：

The best way to write a copy constructor and assignment operator is not to write them at all.

Matrixes with different rows and columns are compatible types. As such it makes a lot of sense to include the dimensions in the template like this:

template <typename T, size_t rows, size_t cols>
class Matrix
{
private:
    T **matrix = nullptr;
}

And now, since rows and cols are know at compile time you can use std::array:

template <typename T, size_t rows, size_t cols>
class Matrix
{
private:
    std::array<std::array<T, rows>, cols> matrix;
}

And hey, now the copy constructor and assignment operator are generated by the compiler by default. Problem solved.

PS: for dynamically sized matrixes use std::vector to the same effect.