Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.println(INVALID_INPUT_ERROR_MESSAGE);
            errorFlag = true;
            return;
        }

        firstMatrix = new double[firstMatrixRows][firstMatrixcolumns];
        secondMatrix = new double[secondMatrixRows][secondMatrixColumns];

        if (firstMatrixRows == 0 || firstMatrixcolumns == 0 || secondMatrixRows == 0 || secondMatrixColumns == 0) {
            System.out.println(INVALID_MATRIX_DIMENSION_ERROR_MESSAGE);
            errorFlag = true;
            return;
        } else if (firstMatrixcolumns != secondMatrixRows) {
            System.out.println(MATRIX_MISMATCH_ERROR_MESSAGE);
            errorFlag = true;
            return;
        }

        resultMatrix = new double[firstMatrixRows][secondMatrixColumns];

    }

    private static void getInput() {

        System.out.println("Enter the first matrix (" + firstMatrixRows + " x " + firstMatrixcolumns + ") :");
        readValues(firstMatrix);

        System.out.println("Enter the second matrix (" + secondMatrixRows + " x " + secondMatrixColumns + ") :");
        readValues(secondMatrix);

    }

    private static void readValues(double matrix[][]) {

        for (int i = 0; i < matrix.length; i++) {
            for (int j = 0; j < matrix[0].length; j++) {
                try {
                    matrix[i][j] = scanner.nextDouble();
                } catch (InputMismatchException ime) {
                    System.out.println(INVALID_INPUT_ERROR_MESSAGE);
                    errorFlag = true;
                    return;
                }
            }
        }

    }

    private static void calculateProduct() {

        for (int i = 0; i < firstMatrixRows; i++) {
            for (int j = 0; j < secondMatrixColumns; j++) {
                for (int k = 0; k < secondMatrixRows; k++) {
                    resultMatrix[i][j] = resultMatrix[i][j] + (firstMatrix[i][k] * secondMatrix[k][j]);
                }
            }
        }

    }

    private static void displayResult() {

        System.out.println("First Matrix:");
        printMatrix(firstMatrix);

        System.out.println("Second Matrix:");
        printMatrix(secondMatrix);

        System.out.println("Result Matrix (Product):");
        printMatrix(resultMatrix);

    }

    private static void printMatrix(double matrix[][]) {

        for (int i = 0; i < matrix.length; i++) {
            for (int j = 0; j < matrix[0].length; j++) {
                System.out.print(decimalFormat.format(matrix[i][j]) + "\t\t");
            }
            System.out.println();
        }
        System.out.println();

    }

}

What are your thoughts on this? Can this code be optimized or done using any other simpler logic? Any suggestions and thoughts are welcome.

share|improve this question
1  
I can't say much of the code, but all the input that you require for a simple multiplication seems to make this very uncomfortable to use (you even request the 'middle' dimension twice). Perhaps you could be inspired by the MATLAB notation, where multiplying 2 matrices is as simple as: [1 2; 3 4]*[1 0; 0 1], or when you are in a verbose mood: mtimes([1 2; 3 4],[1 0; 0 1]). –  Dennis Jaheruddin May 5 at 12:52

1 Answer 1

up vote 17 down vote accepted

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!".

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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