Matrix multiplication in Java

Question

I am taking input from a text file, performing matrix multiplication, then outputting to another text file. I just want to be a better programmer, so please be a detailed as you want. Comments, naming, formatting, etc.

import java.io.*;
import java.util.*;
import java.util.stream.*;

   public class MartixMain {
      public static void main(String [] args){
       BufferedReader br = null;
       BufferedWriter bw = null;
       FileWriter fw = null;
       List<Integer> numbers = new ArrayList<>();
      // This is the rows and columns for the matries
       int row1,column1,row2,column2;
 try {
        Scanner input = new Scanner(new File("C:/Users/jeremiahlukus   /Desktop/New folder/jj.txt")); 
         // adds numbers from .txt to ArrayList numbers
        while(input.hasNextInt()){
         numbers.add(input.nextInt());
        }
      }
 catch(FileNotFoundException e){
          System.err.println("Unable to find the file: fileName");
      }      
          row1 = numbers.get(0);
          column1 = numbers.get(1);
          row2 = numbers.get(2);
          column2 = numbers.get(3);  

          if(column1 != row2){ 
           System.out.println("Column 1 is not equal to row 2. Matrix Multiplication is not possible.");
            try{
              fw = new FileWriter(new File("mytextfile.txt"));
              fw.write(String.format("Column 1 is not equal to row 2. Matrix Multiplication is not possible."));
              fw.close();
            }catch(Exception e){
              System.out.println("NONE");
            }
          }else{  
          //checking to see if they hold the right numbers
          System.out.println("Matrix 1 has " + row1 + " rows and " + column1+ " columns"); 
          System.out.println("Matrix 2 has " + row2 + " rows and " + column2+ " columns"); 
          // This puts the first four numbers as the dimensions of int [][]
          int [][] matrix1 = new int [row1][column1];
          int [][] matrix2 = new int [row2][column2];
         // Removes the first four numbers from the List which are used as dimensions
          numbers.remove(0);
          numbers.remove(0);
          numbers.remove(0);
          numbers.remove(0);
          // This fills the matrix[][]'s up with the numbers in the List
          int iter = 0; // iterates the index of the List
          for( int row = 0; row < matrix1.length; row++){
             for( int col = 0; col < matrix1[row].length;col++){
              matrix1[row][col]= numbers.get(iter);
              iter++; 
             } 
           }
          for( int row = 0; row < matrix2.length; row++){
            for( int col = 0; col < matrix2[row].length;col++){
              matrix2[row][col]= numbers.get(iter);
              iter++;     
            } 
          }
          System.out.println("The elements in Matrix 1 are:");
          System.out.println(Arrays.deepToString(matrix1));
          System.out.println("The elements in Matrix 2 are:");
          System.out.println(Arrays.deepToString(matrix2));

          int[][] multi = multiply(matrix1, matrix2);
          System.out.println("Product of Matrix 1 and Matrix 2 is: ");
          for (int i = 0; i < multi.length; i++) {
            for (int j = 0; j < multi[0].length; j++) {
                  System.out.print(multi[i][j] + " ");
                }
              }
 try{
          fw = new FileWriter(new File("MatrixMultiplication.txt"));
          fw.write(String.format ("Matrix 1 has " + row1 + " rows and " + column1+ " columns")); 
          fw.write(System.lineSeparator()); //new line
          fw.write(String.format ("Matrix 2 has " + row2 + " rows and " + column2+ " columns")); 
          fw.write(System.lineSeparator()); //new line
          fw.write(String.format("The elements in Matrix 1 are:"));
          fw.write(System.lineSeparator()); //new line
          fw.write(String.format(Arrays.deepToString(matrix1)));
          fw.write(System.lineSeparator()); //new line
          fw.write(String.format("The elements in Matrix 2 are:"));
          fw.write(System.lineSeparator()); //new line
          fw.write(String.format(Arrays.deepToString(matrix2)));
          fw.write(System.lineSeparator()); //new line
          fw.write(String.format("Product of Matrix 1 and Matrix 2 is: "));
          fw.write(System.lineSeparator()); //new line

               for (int i = 0; i < multi.length; i++) {
                 for (int j = 0; j < multi[0].length; j++) {
                   fw.write(String.format(multi[i][j] + " "));
                  }
                }                      
              fw.close();
      }catch(IOException ex) {
    ex.printStackTrace();
    }

      }
 }
// This method multiplies the matrices
     public static int[][] multiply(int[][] a, int[][] b)
     {
       int rowsInA = a.length;
       int columnsInA = a[0].length; // same as rows in B
       int columnsInB = b[0].length;
       int[][] c = new int[rowsInA][columnsInB];
       for (int i = 0; i < rowsInA; i++) {
         for (int j = 0; j < columnsInB; j++) {
           for (int k = 0; k < columnsInA; k++) {
              c[i][j] = c[i][j] + a[i][k] * b[k][j]; // Multiplies the matrices together
            }
          }  
       }
       return c; 
     }
}

Input:

3            3
3 4

1         2 3 
4 5 6
7 8 9

1 2 3 4
5 6 7 8
9 10 11 12

Output:

Matrix 1 has 3 rows and 3 columns
Matrix 2 has 3 rows and 4 columns
The elements in Matrix 1 are:
[[1, 2, 3], [4, 5, 6], [7, 8, 9]]
The elements in Matrix 2 are:
[[1, 2, 3, 4], [5, 6, 7, 8], [9, 10, 11, 12]]
Product of Matrix 1 and Matrix 2 is: 
38 44 50 56 83 98 113 128 128 152 176 200

Answer 1

In addition to what Hosch has told you:

Make use of try with resources

You utilize several classes which implement the Closable interface, but aren't explicitly freeing resources you use. Employing a try-with-resources statement will amend that with the benefit of reducing the scope of some variables you currently declare outside.

As an example, rather than the following:

try {
        Scanner input = new Scanner(new File("C:/Users/jeremiahlukus   /Desktop/New folder/jj.txt")); 
         // adds numbers from .txt to ArrayList numbers
        while(input.hasNextInt()){
         numbers.add(input.nextInt());
        }
      }

You can write:

try (Scanner input = new Scanner(
    new File("C:/Users/jeremiahlukus/Desktop/New folder/jj.txt") {
        // logic
}

The newline character

Everywhere you're calling System.lineSeparator() could be replaced with a '\n' newline character preceding where you'd want it.

e.g. This chunk:

fw.write(String.format(Arrays.deepToString(matrix2)));
fw.write(System.lineSeparator()); //new line
fw.write(String.format("Product of Matrix 1 and Matrix 2 is: "));
fw.write(System.lineSeparator()); //new line

Could be just:

  fw.write(String.format(Arrays.deepToString(matrix2)));
  fw.write(String.format("%nProduct of Matrix 1 and Matrix 2 is:%n"));

Be DRY

Anytime you see yourself encountering repition like the following:

numbers.remove(0);
numbers.remove(0);
numbers.remove(0);
numbers.remove(0);

it's a big indication your code could probably do with some refactoring. You could use simple loop:

for (int i = 1; i <= 4; i++) {
  numbers.remove(0);
}

or better yet structured in a method

private static void remove(List<Integer> target, iterations) {
        for (int i = 1; i <= iterations; i++) {
            target.remove(0);
        }
}

which you could invoke, simply, by calling remove(numbers, 4); of course now you have the added benefit of recycling the method elsewhere if necessary.

Don't just catch Exception

While many of us are guilty of just wanting the initial version before changing it, you never want to only catch Exception, for obvious reasons. Be as specific as possible.

Declare variables on separate lines

Let's examine this line: int row1,column1,row2,column2;

Firstly, for the sake of readability you should get in the habit of adding a space after your commas, but even still conventionally each variable should have its on own separate line. It may even seem 'wasteful' to you to do it that way, but the distinct locations make things more readable, flexible and maintainable.

Answer 2

First of all, naming:

int[][] c

This do not tell me what the variables are and what they contain. Most of your naming is OK, although it is best to avoid names like row1, &c.

---

Your formatting could also use some improvement:

      fw.write(System.lineSeparator()); //new line

           for (int i = 0; i < multi.length; i++) {
             for (int j = 0; j < multi[0].length; j++) {
               fw.write(String.format(multi[i][j] + " "));
              }
            }                      
          fw.close();
  }catch(IOException ex) {
ex.printStackTrace();
}

  }

}else{  
//checking to see if they hold the right numbers
System.out.println("Matrix 1 has " + row1 + " rows and " + column1+ " columns"); 

You should keep each level at the same indentation so you can see at a glance what level has what scope.

---

Your comments could also be improved:

fw.write(System.lineSeparator()); //new line

Most people probably know what System.lineSeparator() is.

Also, your comment above multiply() could become a documentation comment. A doc comment is used to supply information to IntelliSense tools and can also be exported to other files if needed. These comments look like:

/**
 * A comment describing the method
 *
 * @param  paramName  A description of a parameter
 * @return            A description of what the method returns
 * @see               Other references to check out
 */

Again, your comments are good for the most part.

---

Have you ever seen the abbreviation SRP? It stands for Single Responsibility Principle, and states that every unit should do only one thing. For example, a method should only do one thing, a class should only do one thing, and so on. Your main() method should start the program and only start the program. Reading the data, working with the data, outputting the data, and so on should all have their own methods that are responsible for, and only responsible for, that. Your multiply() method is good as it is only responsible for multiplying the two matrices.

---

What happens when the input is formatted incorrectly? Does your program handle this use case? While this program may not need to have intensive error handling, many programs do, so this is a point worth mentioning. If the user left out one of the numbers in one of the arrays, it appears your program would throw an exception when you are filling your 2D arrays:

numbers.get(iter);

Answer 3

Swallowing FileNotFoundException without printing its message

When opening the input file, you are swallowing a FileNotFoundException without printing its message.

This exception is also thrown under the following conditions:

• When you try to open a directory as a file
• Opening a file were you don't have read permissions for
• The file doesn't exists

The best way to print a message would be:

catch(FileNotFoundException e){
    System.err.println("Error opening: " + e.getMessage());
}

This would show up as:

Error opening: C:/test.txt (Permission denied)

Answer 4

Formatting

Your formatting (in particular the indentation) is inconsistent, making it hard to see the structure of the code at a glance. You really should be using a code formatter (in eclipse, press Ctrl+Shift+F or configure a save action to format the code whenever you save the file).

Structure

Your main method is rather big. If I have to locate some particular piece of code in there, I have to skim nearly a 100 lines of code. By making shorter methods with clear names, you can make the structure of your code more evident, and permit fast drill-down to any part of the code.

Variables

In Java, it is customary to declare your variables on first use, rather than on top of the method. This makes your code shorter, and more self contained. In particular, it is easier to extract methods because the logical sections of your big method are more self-contained. It also makes it easy to spot unused variables such as br and bw.

Also, avoid assigning default values to local variables (as in br = null). The compiler will emit an error if you use a variable you have not assigned, so it is impossible to run into trouble by using unassigned variables. If you assign a default value, you forgo this safety net.

Finally, some variables have misleading names. I would expect column2 to be the second column, not the number of columns in the second matrix.

Exception Handling

You should close files and streams even if an exception occurs. The try-with-resources statement makes this easy.

Algorithm

First reading the file into an ArrayList is unnecessary.

Example

Combining these, we end up with something like this:

public static void main(String[] args) throws Exception {
    File input = new File("C:/Users/jeremiahlukus   /Desktop/New folder/jj.txt");
    try (Scanner scanner = new Scanner(input)) {
        int rows1 = scanner.nextInt();
        int cols1 = scanner.nextInt();
        int rows2 = scanner.nextInt();
        int cols2 = scanner.nextInt();

        if (cols1 != rows2) {
            throw new IllegalArgumentException("The matrices have incompatible sizes: " 
                    + cols1 + " columns vs. " + rows2 + "rows");
        }

        int[][] matrix1 = readMatrix(scanner, rows1, cols1);
        int[][] matrix2 = readMatrix(scanner, rows2, cols2);

        int[][] product = multiply(matrix1, matrix2);

        System.out.println("The result is:");
        System.out.println(format(product));

        try (PrintWriter pw = new PrintWriter(new File("MatrixMultiplication.txt"))) {
            pw.println("Matrix 1 is the " + rows1 + "x" + cols1 + " matrix");
            pw.print(format(matrix1));
            pw.println("Matrix 2 is the " + rows2 + "x" + cols2 + " matrix");
            pw.print(format(matrix2));
            pw.println("Their product is the " + rows1 + "x" + cols2 + " matrix");
            pw.print(format(product));
        }
    }
}

private static int[][] readMatrix(Scanner scanner, int rows, int cols) {
    int[][] matrix = new int[rows][cols];
    for (int row = 0; row < rows; row++) {
        for (int col = 0; col < cols; col++) {
            matrix[row][col] = scanner.nextInt();
        }
    }
    return matrix;
}

private static int[][] multiply(int[][] a, int[][] b) {
    int rowsInA = a.length;
    int columnsInA = a[0].length; // same as rows in B
    int columnsInB = b[0].length;
    int[][] c = new int[rowsInA][columnsInB];
    for (int i = 0; i < rowsInA; i++) {
        for (int j = 0; j < columnsInB; j++) {
            for (int k = 0; k < columnsInA; k++) {
                c[i][j] = c[i][j] + a[i][k] * b[k][j]; // Multiplies the
                                                        // matrices together
            }
        }
    }
    return c;
}

private static String format(int[][] matrix) {
    StringBuilder sb = new StringBuilder();
    for (int row = 0; row < matrix.length; row++) {
        for (int col = 0; col < matrix[row].length; col++) {
            sb.append(matrix[row][col]);
            sb.append(' ');
        }
        sb.append(System.lineSeparator());
    }
    return sb.toString();
}

Answer 5

In Java and general, having row to row multiplication like

 c[i][j] = c[i][j] + a[i][k] * b[j][k]

is ~5-10 times faster. Just remember in the code to initiate and manave the b like transposed. Also do not modify the address c[i][j] each time and use the variable for summ instead, also maybe try to use a 1-D array, this may give some performance boost.