-1
\$\begingroup\$

This function is for multiplying two matrices together. It's kind of messed up because the result is actually supposed to overwrite the object that called it, or since it's static, it's supposed to overwrite the left object. Is there any way to make it faster or more readable?

public static Matrix multiply(Matrix left, Matrix right){
    int leftRows = left.numRows;
    int leftColumns = left.numCols;
    int rightRows = right.numRows;
    int rightColumns = right.numCols;
    if (leftColumns != rightRows)
        throw new IllegalArgumentException("Dimensions don't match");

    Double[][] result = new Double[leftRows][rightColumns];
    for (int i = 0; i < 2 && i < leftRows; i++) {//I found the && i <leftRows to be necessary for handling matrices of 1 row
        for (int j = 0; j < 2 && j < rightColumns; j++) {//&& j <rightColumns necessary for matrices with 1 columns
            result[i][j] = 0.00000;//ensures no null reference
        }
    }

    for (int i = 0; i < leftRows; i++)
    {
        for (int j = 0; j < rightColumns; j++)
        {
            for (int k = 0; k < leftColumns; k++)
            {
                if(result[i][j] == null)
                    result[i][j] = 0.0;
                result[i][j] += left.getValue(i, k) * right.getValue(k, j);
            }
        }
    }

    Matrix product = new Matrix(leftRows, rightColumns);
    for(int i = 0; i < leftRows; i++)
    {
        for(int j = 0; j < rightColumns; j++)
            Matrix.setValueAt(product, i, j, result[i][j]);
    }
    return product;     

 }
\$\endgroup\$

closed as unclear what you're asking by Simon Forsberg, syb0rg, Malachi, Gareth Rees, Donald.McLean Jan 29 '14 at 15:12

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 8
    \$\begingroup\$ Is this code doing what it is supposed to be? It doesn't sound like it is..... in which case you should take this to Stack Overflow, not here on CodeReview. See the help center for guidance. \$\endgroup\$ – rolfl Jan 29 '14 at 3:47
  • \$\begingroup\$ when you do get it working we would be happy to review the working code. \$\endgroup\$ – Malachi Jan 29 '14 at 14:42
3
\$\begingroup\$

My question would be: Why is it supposed to "overwrite" the left value? This seems dubious from a software engineering point of view. The product of two matrices is: Anm x Bmp = Cnp hence you can only "overwrite" A with C (re-using the same Matrix object) if m == p or in other words if B is a quadratic matrix.

For now let's clean up the existing code by getting rid of the intermediate array

public static Matrix multiply(Matrix left, Matrix right) {
    int leftRows = left.numRows;
    int leftColumns = left.numCols;
    int rightRows = right.numRows;
    int rightColumns = right.numCols;

    if (leftColumns != rightRows)
        throw new IllegalArgumentException("Dimensions don't match");

    Matrix product = new Matrix(leftRows, rightColumns);
    for (int i = 0; i < leftRows; i++)
    {
        for (int j = 0; j < rightColumns; j++)
        {
            double cellValue = 0.0;
            for (int k = 0; k < leftColumns; k++)
            {
                cellValue += left.getValue(i, k) * right.getValue(k, j);
            }
            Matrix.setValueAt(product, i, j, cellValue);
        }
    }

    return product;
 }

You could copy the values back into left - provided right is quadratic and therefore product and left will have the same dimensions. Otherwise if the Matrix class offers a re-sizing option then you could re-size left afterwards and copy product into it. However that is extremely ugly and I would question that requirement.

As a side note: It seems weird that Matrix provides a static method for setting a cell value of a Matrix object. I would have expected that to be an instance method as well like getValue().

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

For improving readability, you could perhaps replace the for-loop variable names with more descriptive names, such as 'currentColumn' and 'currentRow.'

For performance, you can replace 'x++' with '++x' in every loop. 'x++' actually creates a copy of x, increments x, and returns the copy. It is useful in some situations, but unnecessary in a for-loop.

Now I'll admit that I don't work in Java, and Matrixes are a haze from school, but it seems like 'result' could just be set to a Matrix from the start, instead of a double[][]. You would cut down a significant portion of your time spent copying from 'result' to 'product' by just using a Matrix in the first place.

I don't see the need for this:

for (int i = 0; i < 2 && i < leftRows; i++) {//I found the && i <leftRows to be necessary for handling matrices of 1 row
    for (int j = 0; j < 2 && j < rightColumns; j++) {//&& j <rightColumns necessary for matrices with 1 columns
        result[i][j] = 0.00000;//ensures no null reference
    }

Please explain where a null reference might arise without this code.

\$\endgroup\$
  • \$\begingroup\$ The compiler should be able to should be able to know that the result of x++ is not used and optimize it. Even if it doesn't, I wouldn't make the change unless a profiler said it was a bottle neck. result is an array of Double, not the primitive double, so the array is initialized with nulls. I agree that the values should be written directly to product instead of copying them from result. \$\endgroup\$ – unholysampler Jan 29 '14 at 5:55
  • \$\begingroup\$ changing the names of the vars won't help, since the vars are used as column and row indexes in different places. ++x and x++ makes no difference in performance in Java (that can be measured). The array needs to be set non-null because it is an array of Double, not double. \$\endgroup\$ – rolfl Jan 29 '14 at 6:02
  • \$\begingroup\$ The doubles are initialised to null, but won't they all be overwritten? I didn't realise x++ was optimised out, I typically use ++x wherever possible as per link. Just a preference. \$\endgroup\$ – Andrew Williamson Jan 29 '14 at 7:20
  • 1
    \$\begingroup\$ Also, only one downvote on my first answer on Stack Exchange - I think that's not too bad. \$\endgroup\$ – Andrew Williamson Jan 29 '14 at 7:20
  • 1
    \$\begingroup\$ @Andrew In, for example, cellValue += left.getValue(i, k) * right.getValue(k, j); would you make the k the row, or the column? \$\endgroup\$ – rolfl Jan 29 '14 at 11:27

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