4
\$\begingroup\$

I have a 2 dimension matrix :

public struct Matrix2D
{
    private double[,] matrix;

    public Matrix2D(double[,] matrix)
    {
        if (matrix == null) throw new ArgumentNullException(nameof(matrix));

        this.matrix = matrix;
    }

    private double[,] Matrix => matrix ?? (matrix = new double[0, 0]);
    public int RowLength => Matrix.GetLength(1);
    public int ColumnLength => Matrix.GetLength(0);
    public double this[int row, int column] => Matrix[row, column];

    public double[,] AsArray() => (double[,])Matrix.Clone();
}

And a method to sum them :

/// <summary>
/// Sums multiple matrices togheter
/// </summary>
/// <param name="matrices">Matrices to add</param>
/// <returns>New matrix with the summated values</returns>
public static Matrix2D Sum(params Matrix2D[] matrices)
{
    if (matrices == null) throw new ArgumentNullException(nameof(matrices));
    if (matrices.Length == 0) return new Matrix2D();
    if (matrices.Length == 1) return matrices[0];

    int rowLength = matrices[0].RowLength;
    int columnLength = matrices[0].ColumnLength;
    double[,] matrix = matrices[0].AsArray();

    for (int matrixIndex = 1; matrixIndex < matrices.Length; matrixIndex++)
    {
        var matrix2D = matrices[matrixIndex];

        if (matrix2D.RowLength != rowLength || matrix2D.ColumnLength != columnLength)
            throw new ArgumentException("Matrixes must have the same size");

        for (int row = 0; row < rowLength; row++)
        {
            for (int column = 0; row < columnLength; row++)
            {
                matrix[row, column] += matrix2D[row, column];
            }
        }
    }

    return new Matrix2D(matrix);
}

At the moment, the complexity is of \$O(mnp)\$, \$m\$ being the number of matrices and \$n\$ being the row length of matrices and \$p\$ the column length.

I'm wondering if there are edge cases that might crash my method, is it a good decision to make my Matrix2D a struct and is there a way to make this more performant? Obviously any other comments are welcomed.

\$\endgroup\$
  • 1
    \$\begingroup\$ Worth noting, while your struct is immutable, as it really should be, You really should be override the equality on the struct including GetHashCode. This improves performance for the equality. Though i feel I should ask why you chose a struct in this case? It seems like an immutable class would have worked just as well as I don't see any value equality checks anywhere in here and you're not using these for anything odd like dictionary keys by the looks. Generally unless I am in need of value equality for some reason I avoid structs to save from the equality implementation. \$\endgroup\$ – BlindGarret Nov 13 '15 at 19:13
  • \$\begingroup\$ Consider writting an answer about your comment! :) Because there's potential \$\endgroup\$ – IEatBagels Nov 13 '15 at 19:21
  • \$\begingroup\$ Roger, will do. \$\endgroup\$ – BlindGarret Nov 13 '15 at 19:24
3
\$\begingroup\$

A couple of changes I would suggest.

To Struct or not to Struct:

Structs are useful for value equality situations. This is objects which require constant value equality checks or intend to be used as keys in Dictionaries. However what I see in use is more of an immutable class. While structs are considered to be best practice as immutable objects you can do the same with classes, but you get to sidestep the inevitable equality issue.

You see, if you were to use equality on your current Matrix object you will find it is slow to check. In fact structs can even end up using reflection to check equality in the worst case, which is seriously inefficient. see C# - Value Type Equals method - why does the compiler use reflection?

Because of this you should always overload equality when using structs, if only for speed reasons. See What needs to be overriden in a struct to ensure equality operates properly? or one of the many other articles online about this.

Your use case doesn't seem to use or need equality, therefore I would suggest a class over a struct. As a general rule if you don't see yourself using the object as a dictionary key or for equality I wouldn't use a struct.

Var:

On a less important note I am always a strong advocate of using var in your code to make refactoring easier. In the following lines you are declaring the type twice:

int rowLength = matrices[0].RowLength;
int columnLength = matrices[0].ColumnLength;
double[,] matrix = matrices[0].AsArray();
for (int row = 0; row < rowLength; row++){...}

There are actually quite a few more places on top of this. This seems trivial but the usage of var over say double[,] or int (for the case of row) means that if you have to refactor the class into a float[,] or to return a long for a size, that your current code would still function with no effort on your part. Var tends to save on time typing long types and is indispensable for refactoring in looping. It can be a bit debatable, as each team has there own standards, but I can tell you on my team we have an "all var all the time" rule which has been a godsend for refactoring.

{'s:

You swap between style of whether you use a trailing set of {'s between lines. For your for loop you use them, but your if statement does not. While I would advocate always using them, even if you only have a single line in the statement, I would say it's more important to be consistent. If you are going to use the style without {'s for single line statements, do so everywhere. If not, which I generally suggest, do not. This is more of a taste issue, and certainly not a law of the land, but worth noting.

Otherwise this looks good. Hope this helps.

\$\endgroup\$
1
\$\begingroup\$

Your struct is exposing public fields... I'm not all that familiar with C# 6.0 but AFAIK the same nifty one-liner initialization syntax also works for auto-properties, which would be a cleaner implementation than exposing public fields IMO.

The indexer is somewhat lost in the middle of properties public fields, I'd move it just below the constructor, right above the properties public fields (with some whitespace above and below).

I think I'd rename AsArray to ToArray, since it's effectively making a clone, i.e. a copy of the data.

You could make it a class, and reimplement the indexer the verbose way, with a getter and a setter; and then you wouldn't need that AsArray method anymore, would you?

I'm not sure a struct is ideal, but that's only a gut-feeling: I'm not comfortable with a struct that has an array private field, but there's nothing inherently wrong with that. A mutable class that treats matrix values as state seems more intuitive, to me at least.

\$\endgroup\$
  • \$\begingroup\$ Actually, these are properties, not public fields? But I get your point, auto-properties might be a better fit if I kept it as an immutable object. And I think you're right about the mutability, I should consider making the matrix's values "settable" \$\endgroup\$ – IEatBagels Nov 13 '15 at 18:50
  • 2
    \$\begingroup\$ They're properties?? .....I think I hate C# 6.0. \$\endgroup\$ – Mathieu Guindon Nov 13 '15 at 18:51
1
\$\begingroup\$

There are two things I'd consider changing in the code:

  1. Extract the following code in a separate method (which sums only 2 matrices):

    if (matrix2D.RowLength != rowLength || matrix2D.ColumnLength != columnLength)
        throw new ArgumentException("Matrixes must have the same size");
    
    for (int row = 0; row < rowLength; row++)
    {
        for (int column = 0; row < columnLength; row++)
        {
            matrix[row, column] += matrix2D[row, column];
        }
    }
    
  2. Given that the sum of \$n\$ matrices is independent for each couple of matrices you can sum the \$n\$ matrices in groups of 2 in an async way. You just need a queue of Task<Matrix2D> objects.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.