Matrix reloaded ... or reversed

Question

I have written this Matrix class after posting an answer to Malachi's rags-to-riches question. The initial purpose had been to reverse a matrix represented by a 2D array. I have changed it to a more object-orientated version.

Just for fun, I have added the ability to sum two Matrix objects:

public class Matrix
{
    private int[][] internalMatrix;
    private int rowCount = -1;
    private int columnCount = -1;

    private Matrix(int rowCount, int columnCount)
    {
        this.rowCount = rowCount;
        this.columnCount = columnCount;
        Initialize();
    }
    private void Initialize()
    {
        internalMatrix = new int[rowCount][];
        for (int rowIndex = 0; rowIndex < rowCount; rowIndex++)
        {
            internalMatrix[rowIndex] = new int[columnCount];
        }
    }
    public Matrix(int[,] inputMatrix)
    {
        rowCount = inputMatrix.GetLength(0);
        columnCount = inputMatrix.GetLength(1);
        Initialize();

        for (int rowIndex = 0; rowIndex < rowCount; rowIndex++)
        {
            FillRow(rowIndex, inputMatrix);
        }
    }
    private void FillRow(int rowIndex, int[,] inputMatrix)
    {
        for (int columnIndex = 0; columnIndex  < columnCount; columnIndex++)
        {
            this[rowIndex, columnIndex] = inputMatrix[rowIndex, columnIndex];
        }
    }
    public string ToString(String separator)
    {
        StringBuilder sb = new StringBuilder(1024);
        for (int i = 0; i < rowCount; i++)
        {
            sb.AppendLine(String.Join(separator, internalMatrix[i]));
        }
        return sb.ToString();
    }
    public override string ToString()
    {
        return ToString("\t");
    }
    public int[] this[int columnIndex]
    {
        get
        {
            int[] column = new int[rowCount];
            for (int rowIndex = 0; rowIndex < rowCount; rowIndex++)
            {
                column[rowIndex] = this[rowIndex, columnIndex];
            }
            return column;
        }
    }
    public int this[int rowIndex, int columnIndex]
    {
        get { return internalMatrix[rowIndex][columnIndex]; }
        private set { internalMatrix[rowIndex][columnIndex] = value; }
    }
    public Matrix Reverse()
    {
        Matrix matrix = new Matrix(rowCount, columnCount);

        int rowIndex = 0;
        for (int i = rowCount - 1; i >= 0; i--)
        {
            matrix.internalMatrix[i] = GetReversedRowValues(rowIndex);
            rowIndex++;
        }
        return matrix;
    }
    private int[] GetReversedRowValues(int rowIndex)
    {
        int[] reversedRow = new int[columnCount];
        int j = 0;
        for (int i = columnCount - 1; i >= 0; i--)
        {
            reversedRow[j] = this[rowIndex, i];
            j++;
        }
        return reversedRow;
    }
    public static Matrix operator +(Matrix first, Matrix second)
    {
        if (first.rowCount != second.rowCount || first.columnCount != second.columnCount)
        {
            throw new NotSupportedException("For adding 2 matrix they need to have the same row and columcount");
        }

        Matrix sum = new Matrix(first.rowCount, first.columnCount);
        for (int rowIndex = 0; rowIndex < first.rowCount; rowIndex++)
        {
            for (int columnIndex = 0; columnIndex < first.columnCount; columnIndex++)
            {
                sum[rowIndex, columnIndex] = first[rowIndex, columnIndex] + second[rowIndex, columnIndex];
            }
        }
        return sum;
    }
}

And can be called like:

Matrix matrix = new Matrix(new int[,]
    {
        {1, 2, 3},
        {4, 5, 6},
        {7, 8, 9}
    }
    );

Matrix reversedMatrix = matrix.Reverse();
Console.WriteLine(matrix.ToString());
Console.WriteLine(reversedMatrix.ToString());
Console.WriteLine((matrix + reversedMatrix).ToString());

or

var matrix2d = new[,]
    {
        { 1,  2,  3,  4},
        { 5,  6,  7,  8},
        { 9, 10, 11, 12}
    };

Matrix matrix = new Matrix(matrix2d);
Matrix reversedMatrix = matrix.Reverse();
Matrix summedMatrix = marix + reversedMatrix;

Console.WriteLine(matrix.ToString(" "));
Console.WriteLine(reversedMatrix.ToString(" "));
Console.WriteLine(summedMatrix.ToString(" "));

This works like a charm. Is there anything to improve?

Answer 1

A few things I don't like:

The use of private fields instead of public properties with protected setter. Doing so will make your class more extensible and usable by others:

public int RowCount {get;protected set;}

The existance of FillRow isn't really justified - it's an extra method call for no apparent reason. Two nested loops should be just fine here

Reversing shouldn't require the extra variables

private int[] GetReversedRowValues(int rowIndex)
    {
        int[] reversedRow = new int[columnCount]; 
        for (int i = 0; i < ColumnCount; i++)
        {
            reversedRow[i] = this[rowIndex, ColumnCount - i]; 
        }
        return reversedRow;
    }

or even be implemented through array copy or yield - you have too many operations that just copy one and the same data back and forth, not very efficient at all.

If the major feature of your class is reversing the matrix you can just keep a "reversed" bool flag and change the loop bounds and directions instead of copying data.

Answer 2

to make this a little clearer you can declare more than one incrementation variable inside your for loop

private int[] GetReversedRowValues(int rowIndex)
{
    int[] reversedRow = new int[columnCount];
    int j = 0;
    for (int i = columnCount - 1; i >= 0; i--)
    {
        reversedRow[j] = this[rowIndex, i];
        j++;
    }
    return reversedRow;
}

like this

private int[] GetReversedRowValues(int rowIndex)
{
    int[] reversedRow = new int[columnCount];
    for (int i = columnCount - 1, j = 0; i >= 0; i--, j++)
    {
        reversedRow[j] = this[rowIndex, i];
    }
    return reversedRow;
}

I don't know what the standard on doing this is, but it seems like a good idea especially for reversing an array.

Answer 3

I think you should space your methods, having them one after another like this is hard to read I think.

Also, your methods, constructors and indexers are mixed up in your class, I think you should try to group them (not by using regions of course), it would be easier to follow the class.

I have nothing else to say, OOP seems good, same thing for the matrix methods themselves.