A Java class that prints a matrix

Question

I am trying to learn some generic programming with Java:

import java.util.ArrayList;
import java.util.HashSet;
import java.util.Set;

/**
 * Created by PACKAGE_NAME on 15/11/14.
 */
public class PrintMatrix {
    public static <T> void printma(ArrayList<ArrayList<T>> matrix) {
        ArrayList<Integer> sizes = new ArrayList<>();
        for(ArrayList<T> row : matrix) {
            sizes.add(row.size());
        }
        Set<Integer> sizesSet = new HashSet<>(sizes);
        Integer[] sizesArray = sizesSet.toArray(new Integer[0]);
        Integer nCol = sizesArray[0];
        Integer nRow = matrix.size();

        if(sizesSet.size() > 1) {
            System.err.println("This is not a matrix.");
            return;
        }

        for(int i=0; i<nRow; i++) {
            for(int j=0; j<nCol; j++) {
                System.out.printf("%s\t", matrix.get(i).get(j).toString());
            }
            System.out.println();
        }
    }

    public static <T> void printmb(ArrayList<T[]> matrix) {
        ArrayList<Integer> sizes = new ArrayList<>();
        for(T[] row : matrix) {
            sizes.add(row.length);
        }
        Set<Integer> sizesSet = new HashSet<>(sizes);
        Integer[] sizesArray = sizesSet.toArray(new Integer[0]);
        Integer nCol = sizesArray[0];
        Integer nRow = matrix.size();

        if(sizesSet.size() > 1) {
            System.err.println("This is not a matrix.");
            return;
        }

        for(int i=0; i<nRow; i++) {
            for(int j=0; j<nCol; j++) {
                System.out.printf("%s\t", matrix.get(i)[j].toString());
            }
            System.out.println();
        }
    }
}

rolfl · Accepted Answer · 2014-11-15 21:55:51Z

Your two print methods are odd in the sense that they are inconsistent with how you present them. A 'classic' matrix consists of primitive arrays (like int[][]), and not nested containers (like ArrayList<ArrayList<T>>). I can understand why you have one and also the other, but the mix of them is bizarre, ArrayList<T[]>. Remove the second print method.

Let's go through some other items first though. Let's narrow the ArrayList down to the interface level, just List. There is no reason to constrain the method to the concrete implementation because that severely restricts the applicability. I would actually go further and constrain it to Iterable. This produces the signature:

public static <T, R extends Iterable<T>, M extends Iterable<R>> void printMatrix(M matrix) {
    ....
}

What am I saying there? I am saying that the Matrix can consist of any combination of values that may have an Iterator. The mnemonics I use there are that:

T is the type of the cell data
R is the type of each row (which can be anything that is of type Iterable<T>)
M is the combined Matrix, which can be any Iterable of <R> (i.e. it's a collection of rows)

This is a generic method. You can throw pretty much anything at it from any collection structure.

An implementation of the method would look something like:

public static <T, R extends Iterable<T>, M extends Iterable<R>> void printMatrix(M matrix) {
    int size = -1;
    for (R row : matrix) {
        int s = 0;
        for (@SuppressWarnings("unused") T value : row) {
            s++;
        }
        if (size < 0) {
            size = s;
        } else if (size != s) {
            System.err.println("This is not a matrix.");
            return;
        }
    }

    StringBuilder sb = new StringBuilder();
    for (R row : matrix) {
        sb.setLength(0);
        for (T value : row) {
            sb.append(value).append('\t');
        }
        if (sb.length() > 0) {
            sb.setLength(sb.length()-1);
        }
        System.out.println(sb);
    }
}

and this could be used, for example, like:

public static void main(String[] args) {
    List<Collection<Object>> data = new ArrayList<>();
    data.add(System.getProperties().keySet());
    data.add(System.getProperties().values());
    printMatrix(data);
}

You should note a few things here:

The only reference to any concrete class here is the new ArrayList<>() in the main method. Everything else is treated at the lowest usable interface level (Collections, Lists, Iterables).
I use StringBuilder to build things 1 line at a time. This improves performance because each print is expensive, so you do fewer prints.

Java8 has a better way of counting the members, and also joining the Strings with the \t character:

for (R row : matrix) {
    long s = StreamSupport.stream(row.spliterator(), false).count();
    if (size < 0) {
        size = s;
    } else if (size != s) {
        System.err.println("This is not a matrix.");
        return;
    }
}

for (R row : matrix) {
    StringJoiner sj = new StringJoiner("\t");
    row.forEach(v -> sj.add(String.valueOf(v)));
    System.out.println(sj);
}

There is no need for the array-of-Integer to count the number of values in each line.
By putting the complexity of the generic types in the static method generics declaration area, the actual parameter has the simple type M.

janos · Accepted Answer · 2014-11-15 22:16:09Z

A few additional points on top of what others have already said.

Input validation

Printing an error message to the console is not the right way to handle invalid input. The right way is to throw IllegalArgumentException, for example:

if(sizesSet.size() > 1) {
    throw new IllegalArgumentException("This is not a matrix.");
}

Also, the way you implemented checking for a valid matrix is both tedious and inefficient. It's tedious because you build a ArrayList of sizes, then convert that to a HashSet of sizes. You could simplify that a lot, following this logic:

Get the size of the first row (= number of columns of first row)
Iterate over the remaining rows, and if the size doesn't match, throw

Like this:

int nCol = matrix.get(0).size();
for (int i = 1; i < matrix.size(); ++i) {
    if (nCol != matrix.get(i).size()) {
        throw new IllegalArgumentException("This is not a matrix.");
    }
}

Finally, make sure to check for valid input before doing anything else. For example, looking at this:

Integer[] sizesArray = sizesSet.toArray(new Integer[0]);
Integer nCol = sizesArray[0];
Integer nRow = matrix.size();

if(sizesSet.size() > 1) {
    System.err.println("This is not a matrix.");
    return;
}

Notice that the 3 lines before the if statement were completely unnecessary to do if the parameter is invalid and you will return from the method anyway.

Avoid `Integer` when `int` is all you need

If all you need is an int, then don't use an Integer. For example here:

Integer nRow = matrix.size();

An int would have been enough.

Use for-each loop

If you don't need the item indexes, then it's better to use a for-each loop.

For example instead of this:

for(int i=0; i<nRow; i++) {
    for(int j=0; j<nCol; j++) {
        System.out.printf("%s\t", matrix.get(i).get(j).toString());
    }
    System.out.println();
}

It's simpler this way:

for (List<T> row : matrix) {
    for (T value : row) {
        System.out.printf("%s\t", value);
    }
    System.out.println();
}

lealand · Accepted Answer · 2014-11-15 21:26:58Z

Let's go through this line by line:

public static <T> void printma(ArrayList<ArrayList<T>> matrix) {

First of all, printma? What kind of method name is that? Calling code would then be PrintMatrix.printma(....). Since you have it in the PrintMatrix class already, just call it print or display.

Also, why are you demanding that you the input array be and ArrayListof ArrayLists? Just make it List<List<T>> matrix, though I think an array might be better.

 ArrayList<Integer> sizes = new ArrayList<>();

Again, demanding an ArrayList. Should be List<Integer> sizes = new ArrayList<>(), but now I'm getting more concerned. I always do when I see someone wrapping primitive types in a collection. Still thinking we should just be working with arrays here.

    Set<Integer> sizesSet = new HashSet<>(sizes);
    Integer[] sizesArray = sizesSet.toArray(new Integer[0]);
    Integer nCol = sizesArray[0];
    Integer nRow = matrix.size();

I have no idea what's going on here. You just populated the sizes list before hand, and did only to populate the sizesSet object you made afterwards. There was no need to do the first step!

And why all the Integer objects? Just make them ints. E.g. int[] sizesArray.

But then I go onto the next line and discover that the only reason you created is for the next line!

Long story short, this:

public static <T> void printma(ArrayList<ArrayList<T>> matrix) {
    ArrayList<Integer> sizes = new ArrayList<>();
    for(ArrayList<T> row : matrix) {
        sizes.add(row.size());
    }
    Set<Integer> sizesSet = new HashSet<>(sizes);
    Integer[] sizesArray = sizesSet.toArray(new Integer[0]);
    Integer nCol = sizesArray[0];
    Integer nRow = matrix.size();

is equivalent to

public static <T> void printma(List<List<T>> matrix) {
  int cols = matrix.get(0).size();
  int rows = matrix.size();

ferada · Accepted Answer · 2014-11-15 21:18:46Z

This seems a bit complicated for what it does even though obviously the use of generics is appropriate for a generalised matrix.

First of all, both printma and printmb are almost the same. Now if the implied question is which of both options is better, then the answer is neither, since in OOP languages it is almost certainly assumed to have proper objects to operate on, in this case you'd want a Matrix<T> class first to take care of storage, invariants and then implement either a toString method, or if necessary even a PrintMatrix helper to do that job. In either case the methods should probably also return a string instead, or have a stream parameter to print the matrix on.

If you implement a matrix class, you'll most likely run into issues with generics the way you use nested arrays at the moment. If, for example, you stay with two-dimensional matrixes, you more easily store all elements in a continuous array and calculate the index of an element from their (x,y) coordinate by using width and height of the matrix. That way you don't have to allocate that many elements and simplify a lot of your book-keeping.

Code-wise, the test with the set is clever but not very efficient. You could just iterate over all sizes and exit if one of them isn't the same. (If you had a matrix class you couldn't get the error anyway.)

The actual printing loop would error out if one of the elements is set to null. Similary to Set and HashSet (which is good), you can do the same for List and ArrayList.

Pharap · Accepted Answer · 2014-11-16 10:09:42Z

Most of the issues have been pretty much covered by everyone else, but I have one last thing to mention.

This line:

Integer[] sizesArray = sizesSet.toArray(new Integer[0]);

is wasteful. That 0-length array is allocated on the heap, and then just scrapped without being used. It's not a big problem in this case, but it's not good either.

This on the other hand:

Integer[] sizesArray = sizesSet.toArray(new Integer[sizesSet.size()]);

Won't waste the new array. The toArray method recognises that the array passed in is large enough to hold all the elements in the ArrayList, so instead of allocating a new array (which it would do if you gave it an array that was too small) it simply fills the array given to it. Again, it's not a major issue in this case, but it's still important.

Stack Exchange Network

A Java class that prints a matrix

5 Answers 5

Input validation

Avoid `Integer` when `int` is all you need

Use for-each loop

You must log in to answer this question.

Hot Network Questions

A Java class that prints a matrix

5 Answers 5

Input validation

Avoid Integer when int is all you need

Use for-each loop

You must log in to answer this question.

Related

Hot Network Questions

Avoid `Integer` when `int` is all you need