HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Matrix multiplication

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
matrixmultiplicationstackoverflow

Problem

Below is the code that I've written for matrix multiplication:

```
import java.text.DecimalFormat;
import java.util.InputMismatchException;
import java.util.Scanner;

public class MatrixMultiplication {

private static final String TERMINATED_MESSAGE = "Terminated" + "," + " ";
private static final String INVALID_MATRIX_DIMENSION_ERROR_MESSAGE = TERMINATED_MESSAGE + "Invalid matrix dimension!";
private static final String MATRIX_MISMATCH_ERROR_MESSAGE = TERMINATED_MESSAGE + "First matrix column and second matrix row must be same!";
private static final String INVALID_INPUT_ERROR_MESSAGE = TERMINATED_MESSAGE + "Invalid Input!";

private static Scanner scanner;
private static DecimalFormat decimalFormat;

private static int firstMatrixRows;
private static int firstMatrixcolumns;
private static int secondMatrixRows;
private static int secondMatrixColumns;

private static double firstMatrix[][];
private static double secondMatrix[][];
private static double resultMatrix[][];

private static boolean errorFlag;

public static void main(String[] args) {

initialize();
if (!errorFlag) getInput();
if (!errorFlag) calculateProduct();
if (!errorFlag) displayResult();

}

private static void initialize() {

scanner = new Scanner(System.in);
decimalFormat = new DecimalFormat("#.##");

errorFlag = false;

try {
System.out.print("Number of Rows in First Matrix: ");
firstMatrixRows = scanner.nextInt();

System.out.print("Number of Columns in First Matrix: ");
firstMatrixcolumns = scanner.nextInt();

System.out.print("Number of Rows in Second Matrix: ");
secondMatrixRows = scanner.nextInt();

System.out.print("Number of Columns in Second Matrix: ");
secondMatrixColumns = scanner.nextInt();
} catch (InputMismatchException ime) {
System.out

Solution

Static and void

All your variables are static. All your methods return void. This is not good.

Java is an object-oriented language. You're not using it that way. You're using it more as a procedural language by having everything as static and using only void methods. Although this works (apparently), you're losing flexibility.

I have a mission for you:

Remove all the below lines from your program and use them as local variables rather than static fields.

private static Scanner scanner;
private static DecimalFormat decimalFormat; 

private static int firstMatrixRows;
private static int firstMatrixcolumns;
private static int secondMatrixRows;
private static int secondMatrixColumns;

private static double firstMatrix[][];
private static double secondMatrix[][];
private static double resultMatrix[][];

private static boolean errorFlag;


To get you started I have some few suggestions:

  • displayResult method can take three parameters: firstMatrix, secondMatrix, and, you guessed it: resultMatrix.



  • calculateProduct can take two parameters: firstMatrix, secondMatrix, and return the result matrix.



  • getInput can be modified into only reading one matrix, then you can use getInput("Enter the first matrix", firstMatrix);



  • Use firstMatrix.length and firstMatrix[0].length to determine the width and height of a matrix.



Your own Matrix class

And finally, this is a major suggestion that partially goes against the above suggestions:

Replace double[][] with MyMatrix that you create as your own class.

  • The MyMatrix class itself can contain the getInput method and a public MyMatrix multiply(MyMatrix otherMatrix) method.



  • It can also contain double[][] matrixData



  • The class can contain an output method for outputting it's internal matrixData.



  • It can also contain, if you want, private final int columns; and private final int rows;



This is what I would ultimately recommend, as it will allow you to add an add method, and a whole lot of other matrix-specific methods such as calculateInverseMatrix.

Constants

The only variables I can accept being static are the ones marked final (the constants). I would however make a minor change to one of them, as there's no meaning to use string concatenation here: "Terminated" + "," + " ".

private static final String TERMINATED_MESSAGE = "Terminated, ";


Use exceptions rather than errorFlag

if (firstMatrixRows == 0 || firstMatrixcolumns == 0 || secondMatrixRows == 0 || secondMatrixColumns == 0) {
        throw new IllegalStateException(INVALID_MATRIX_DIMENSION_ERROR_MESSAGE);
    } else if (firstMatrixcolumns != secondMatrixRows) {
        throw new IllegalStateException(MATRIX_MISMATCH_ERROR_MESSAGE);
    }


Throwing an exception is to be used for exceptional cases. You should perhaps consider creating your own Exception class, and ask yourself if you want it to be a checked or unchecked exception.

Once you have created your own MyMatrix class and restructured your program a bit to use more object orientation (remember my challenge, get rid of all those static variables!) I hope that you will write a follow-up question and that I will say "Well done! You did it!".

Code Snippets

private static Scanner scanner;
private static DecimalFormat decimalFormat; 

private static int firstMatrixRows;
private static int firstMatrixcolumns;
private static int secondMatrixRows;
private static int secondMatrixColumns;

private static double firstMatrix[][];
private static double secondMatrix[][];
private static double resultMatrix[][];

private static boolean errorFlag;
private static final String TERMINATED_MESSAGE = "Terminated, ";
if (firstMatrixRows == 0 || firstMatrixcolumns == 0 || secondMatrixRows == 0 || secondMatrixColumns == 0) {
        throw new IllegalStateException(INVALID_MATRIX_DIMENSION_ERROR_MESSAGE);
    } else if (firstMatrixcolumns != secondMatrixRows) {
        throw new IllegalStateException(MATRIX_MISMATCH_ERROR_MESSAGE);
    }

Context

StackExchange Code Review Q#48969, answer score: 17

Revisions (0)

No revisions yet.