Immutable matrix class with mathematical methods

Question

This was a homework assignment for a data structures course. The goal was to implement a given interface and the assignment was auto-graded by a system that ran some validation tests. Sadly I did not really get any feed back on the code structure or any advice on how to improve on what I did if anything was wrong or non-idiomatic for java, a language I have not really use outside of this course.

I am particularly interested in if my code for handling the math operations is silly or "overly clever" or maybe if there is a better way I could have achieved a similar pattern that would be more idiomatic for someone well versed in java.

interface MathOperation
{
    int Run(int a, int b);
}

interface MatrixOperation
{
    void Run(int row, int column);
}

public class CompletedMatrix implements Matrix
{

    private int[][] _matrix;

    public CompletedMatrix(int[][] matrix)
    {
        if (!NullCheck(matrix))
        {
            throw new IllegalArgumentException("Cannot perform operation on null matrix");
        }
        _matrix = new int[matrix.length][];

        for (int row = 0; row < matrix.length; row++)
        {
            _matrix[row] = matrix[row].clone();
        }
    }

    @Override
    public int getElement(int y, int x)
    {
        return _matrix[y][x];
    }

    @Override
    public int getRows()
    {
        return _matrix.length;
    }

    @Override
    public int getColumns()
    {
        int columns = 0;

        if (this.getRows() != 0)
        {
            columns = _matrix[0].length;
            for (int row = 1; row < this.getRows(); row++)
            {
                if (columns != _matrix[row].length)
                {
                    throw new RuntimeException("Matrices do not have matching dimensions");
                }
            }
        }

        return columns;
    }

    @Override
    public Matrix plus(Matrix other)
    {
        return ExecuteMatrixMathOperation(other, (a, b) -> a + b);
    }

    @Override
    public Matrix minus(Matrix other)
    {
        return ExecuteMatrixMathOperation(other, (a, b) -> a - b);
    }

    @Override
    public Matrix multiply(Matrix other)
    {
        return ExecuteMatrixMathOperation(other, (a, b) -> a * b);
    }

    private Matrix ExecuteMatrixMathOperation(Matrix b, MathOperation op)
    {
        ValidateMatrix(b);

        int[][] result = new int[this.getRows()][this.getColumns()];
        MatrixOperation((row, column) ->
        {
            result[row][column] = op.Run(this.getElement(row, column), b.getElement(row, column));
        });

        return new CompletedMatrix(result);
    }

    private void ValidateMatrix(Matrix other)
    {
        if (!NullCheck(other))
        {
            throw new IllegalArgumentException("Cannot perform operation on null matrix");
        }

        if (!DimensionsMatch(other))
        {
            throw new RuntimeException("Matrices do not have matching dimensions");
        }
    }

    private boolean NullCheck(Object other)
    {
        return (this != null && other != null);
    }

    private boolean DimensionsMatch(Matrix other)
    {
       return (this.getRows() == other.getRows() && this.getColumns() == other.getColumns());
    }

    private void MatrixOperation(MatrixOperation op)
    {
        for(int row = 0; row < this.getRows(); row++)
        {
            for(int column = 0; column < this.getColumns(); column++)
            {
                op.Run(row, column);
            }
        }
    }
}

This is a trimmed down interface given in the assignment

public interface Matrix 
{
    public int getElement(int y, int x);
    public int getRows();
    public int getColumns();
    public Matrix plus(Matrix other);
    public Matrix minus(Matrix other);
    public Matrix multiply(Matrix other);
}

Answer 1

In canonical java, Methods use camelCase. This visually distinguishes them from classes, which start with a capital letter. Open parens are on the same line, not a new line Underscores are not used to indicate instance variables There is whitespace between a control flow keyword (for, if, while, do) and the open paren. This visually distinguishes it from a method call.

The method nullCheck is just overhead. this != null will always be true, so the only part that matters is other != null. High preferable would be Objects.requireNonNull().

The exception message in the constructor is confusing. Constructing an objects is not "perform[ing] an operation".

getColumns is the wrong place to validate the columns are correct. What if the client passes in a malformed matrix and then never calls getColumns? Validating the invariants of a class belongs in the constructor to that class. This check can be folded into the copying of the rows.

When throwing exceptions, try to provide enough information in the exception message to debug. For instance, don't just say there's a mismatch in column lengths, also call out the rows which don't match.

The matrix parameter starts as "other", which I'm not a big fan of, and then inexplicably becomes b in executeMatrixMathOperation. This is confusing on its own, and made worse by the use of b in the anonymous function definitions in plus, minus, and multiply. I'd really rather the parameter was consistently named matrix, or perhaps otherMatrix. other isn't meaningful, and neither is b.

When an argument is invalid, prefer an IllegalArgumentException over the more generic RuntimeException.

dimensionsMatch can be folded into validateMatrix. There's not enough there to be its own method.

op is inadequately descriptive. mathOperation or matrixOperation would be preferable, especially since the same parameter name is reused for both kinds of operation.

You don't "run" a mathematical operation, you evaluate it.

The matrixOperation method implementation can be moved into executeMatrixMathOperation. The extra complexity is not worth the method being 2 lines shorter.

I'm not super fond of being able to declare a new CompletedMatrix(new int[0][5]), then calling getColumns() and getting back 0. If this is the expected behavior, it should be documented.

If you made all these changes, your code might look more like:

import java.util.Objects;

public final class CompletedMatrix implements Matrix {
    
    private final int[][] matrix;

    public CompletedMatrix(int[][] matrix) {
        Objects.requireNonNull(matrix, "`matrix` parameter may not be null.");
        
        if (matrix.length == 0) {
            this.matrix = new int[0][0];
            return;
        }
        
        this.matrix = new int[matrix.length][];
        int columns = matrix[0].length;
        for (int row = 0; row < matrix.length; row++) {
            this.matrix[row] = matrix[row].clone();
            if (matrix[row].length != columns) {
                throw new IllegalArgumentException("All rows must have the same number of columns. Row 0 has " 
                        + columns + " columns, but row " + row + " has " + matrix[row].length);
            }
        }
    }

    @Override
    public int getElement(int y, int x) {
        return matrix[y][x];
    }

    @Override
    public int getRows() {
        return matrix.length;
    }

    @Override
    public int getColumns() {
        return (matrix.length == 0) ? 0 : matrix[0].length;
    }

    @Override
    public Matrix plus(Matrix otherMatrix) {
        return executeMatrixMathOperation(otherMatrix, (a, b) -> a + b);
    }

    @Override
    public Matrix minus(Matrix otherMatrix) {
        return executeMatrixMathOperation(otherMatrix, (a, b) -> a - b);
    }

    @Override
    public Matrix multiply(Matrix otherMatrix)  {
        return executeMatrixMathOperation(otherMatrix, (a, b) -> a * b);
    }

    private Matrix executeMatrixMathOperation(Matrix otherMatrix, MathOperation mathOperation) {
        validateMatrix(otherMatrix);

        int[][] result = new int[this.getRows()][this.getColumns()];
        for (int row = 0; row < this.getRows(); row++)  {
            for (int column = 0; column < this.getColumns(); column++) {
                result[row][column] = mathOperation.evaluate(this.getElement(row, column), otherMatrix.getElement(row, column));
            }
        }

        return new CompletedMatrix(result);
    }

    private void validateMatrix(Matrix otherMatrix) {
        Objects.requireNonNull(otherMatrix, "`otherMatrix` parameter may not be null.");
        
        if ((this.getRows() != otherMatrix.getRows()) || (this.getColumns() != otherMatrix.getColumns())) {
            throw new IllegalArgumentException("Cannot operate on matrixes of different sizes. This matrix has "
                    + this.getRows() + " rows and " + this.getColumns() + " columns, while the argument matrix has "
                    + otherMatrix.getRows() + " rows and " + otherMatrix.getColumns() + "columns");
        }
    }
}

Answer 2

MathOperation and MatrixOperation are pretty much functional interfaces. You should follow similar naming as used in BiFunction and BiConsumer and use apply instead of run. The run method is very closely associated to Thread and Runnable and using it in a different context is confusing.

Follow common Java naming conventions: always start method names with lower case letter.

CompletedMatrix(...) throws an IllegalArgumentException if you pass a null reference to the constructor. It should throw a NullPointerException instead. The IllegalArgumentException should be reserved for cases like passing a 2D array that has different length sub-arrays (which you should check).

The getColumns() returns the number of columns. I know it's named by your professor, but they should have named it getColumnCount() instead. Because the method name is in plural, I expect it to return a collection of columns, not just a single integer. This kind of things increase the cognitive load on the reader and makes the code hard to follow. Your implementation of this method also does the matrix internal state validation that should have been done in the constructor. It's better to perform error checking in the constructor and just prevent the user from creating objects with invalid state.

ExecuteMatrixMathOperation(...) has unnecessary curly braces in the lambda operation.

ValidateMatrix(...) throws an IllegalArgumentException when it should throw a NullPointerException and a RuntimeException when it should throw an IllegalArgumentException. Throwing a bare RuntimeException is always a code smell and you should look into better alternatives.

Checking this in NullCheck(...) is redundant. The this reference can never be null.

MatrixOperation(...) loops through every element in the matrix and for every value it calls getColumns(), which loops through every row because it does the error checking. It's computationally quite an expensive operation.

Answer 3

Fairly overlapping with the other answers:

MathOperation is really just a BiFunction. If you want to alias it, you can inherit from it.

NullCheck is... not that useful unless it's done basically everywhere, and the best way to do that is to use a library that supports adding @NotNull to parameters. Newer languages like C# have this built in.

I don't see the point of cloning your input array. You already make a new input array in your calculations. It would be fine to simply document that input arrays are not cloned at the point of construction; otherwise this is going to be an expense you need to pay even if it isn't necessary.

Consider using the stream library. If you end up needing to support massive matrices, (first use a third-party library written specifically for this, but otherwise) enabling parallel computation will be easy.

Consider adding printing methods.

Suggested

import java.util.Arrays;
import java.util.function.BiFunction;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

public class Main {
    public interface Matrix
    {
        int getElement(int y, int x);
        int getRows();
        int getColumns();
        Matrix plus(Matrix other);
        Matrix minus(Matrix other);
        Matrix multiply(Matrix other);
    }

    public static class CompletedMatrix implements Matrix
    {
        private interface MathOperation extends BiFunction<Integer, Integer, Integer> { }

        private final int[][] data;

        private CompletedMatrix(int[][] data) {
            // Fast path for pre-validated data
            this.data = data;
        }

        public static CompletedMatrix fromArray(int[][] array) {
            if (isJagged(array))
                throw new IllegalArgumentException("Input matrix is jagged");
            return new CompletedMatrix(array);
        }

        private static boolean isJagged(int[][] array) {
            return Arrays.stream(array)
                .anyMatch(row -> row.length != array[0].length);
        }

        public int getElement(int y, int x) {
            return data[y][x];
        }

        public int getRows() {
            return data.length;
        }

        public int getColumns() {
            return data[0].length;
        }

        public Matrix plus(Matrix other) {
            return execute(other, Integer::sum);
        }

        public Matrix minus(Matrix other) {
            return execute(other, (a, b) -> a - b);
        }

        public Matrix multiply(Matrix other) {
            return execute(other, (a, b) -> a * b);
        }

        private CompletedMatrix execute(Matrix genericOther, MathOperation op) {
            CompletedMatrix other = (CompletedMatrix)genericOther;
            if (!dimensionsMatch(other))
                throw new RuntimeException("Matrices do not have matching dimensions");

            int[][] result = IntStream.range(0, data.length)
                .mapToObj(i -> zipMap(data[i], other.data[i], op))
                .toArray(int[][]::new);

            return new CompletedMatrix(result);
        }

        private static int[] zipMap(int[] a, int[] b, MathOperation op) {
            return IntStream.range(0, a.length)
                .map(j -> op.apply(a[j], b[j]))
                .toArray();
        }

        public boolean dimensionsMatch(Matrix other) {
            return getRows() == other.getRows()
                && getColumns() == other.getColumns();
        }

        @Override
        public String toString() {
            return Arrays.stream(data)
                .map(
                    row -> "["
                    + Arrays.stream(row)
                        .mapToObj("%2d"::formatted)
                        .collect(Collectors.joining(", "))
                    + "]"
                )
                .collect(Collectors.joining("\n"));
        }
    }

    public static void main(String[] args) {
        CompletedMatrix
            a = CompletedMatrix.fromArray(new int[][] {
                {1, 2},
                {3, 4},
                {5, 6},
            }),
            b = CompletedMatrix.fromArray(new int[][] {
                {9, 6},
                {8, 5},
                {7, 4},
            });

        System.out.printf("%s%n+%n%s%n=%n%s%n", a, b, a.plus(b));
    }
}

Output

[ 1,  2]
[ 3,  4]
[ 5,  6]
+
[ 9,  6]
[ 8,  5]
[ 7,  4]
=
[10,  8]
[11,  9]
[12, 10]