views:

94

answers:

5
public class Matrix
{

    public static int rows;
    public static int colms;//columns
    public static int[][] numbers;

    public Matrix(int[][] numbers)
    {

        numbers = new int[rows][colms];

    }


    public static boolean isSquareMatrix(Matrix m)
    {
        //rows = numbers.length;
        //colms = numbers[0].length;

        if(rows == colms)
           return true;
        else
            return false;
    }

    public static Matrix getTranspose(Matrix trans)
    {
       trans = new Matrix(numbers);

        for(int i =0; i < rows; i++)
        {
            for(int j = 0; j < colms; j++)
            {
                trans.numbers[i][j] = numbers[j][i];
            }
        }
        return trans;
    }



    public static void main(String[] args)
    {
        int[][] m1 = new int[][]{{1,4}, {5,3}};
        Matrix Mat = new Matrix(m1);

        System.out.print(Mat);
        System.out.print(getTranspose(Mat));

    }
}
+7  A: 

You need to implement toString() in a meaningful way.

This toString() (below) is perhaps suitable for debugging, but will be ugly and confusing if you use it for real user output. An actual solution would probably use a Formatter in some complicated way to produce neatly tabular rows and columns.

Some additional recommendations based on your code:

  • Suggest not storing the rows/columns sizes separately. SSOT / Single Source of Truth or DRY, Java+DRY. Just use the .length, and provide accessor methods if need be.
  • Use final in method args, it will eliminate bugs like you have above, aliasing numbers incorrectly int the constructor
  • Use an instance, not static
  • Paranoia is the programmer's lifestyle: I also modified my code to do a deepCopy of the provided int[][] array, otherwise there is reference leakage, and the Matrix class would be unable to enforce its own invariants if caller code later modified the int[][] they passed in.

  • I made my Matrix immutable (see final private numbers[][]) out of habit. This is a good practice, unless you come up with a good reason for a mutable implementation (wouldn't be surprising for performance reasons in matrices).

Here's some improved code:

public final class Matrix
{
    final private int[][] numbers;

    // note the final, which would find a bug in your cited code above...
    public Matrix(final int[][] numbers)
    {   
        // by enforcing these assumptions / invariants here, you don't need to deal 
        // with checking them in other parts of the code.  This is long enough that you might 
        // factor it out into a private void sanityCheck() method, which could be 
        // applied elsewhere when there are non-trivial mutations of the internal state

        if (numbers == null || numbers.length == 0) 
          throw new NullPointerException("Matrix can't have null contents or zero rows");
        final int columns = numbers[0].length;
        if (columns == 0) 
          throw new IllegalArgumentException("Matrix can't have zero columns");
        for (int i =1; i < numbers.length; i++) {
          if (numbers[i] == null) 
             throw new NullPointerException("Matrix can't have null row "+i);
          if (numbers[i].length != columns) 
             throw new IllegalArgumentException("Matrix can't have differing row lengths!");
        }
        this.numbers = deepCopy(numbers);
    }

    public boolean isSquareMatrix() { return rowCount() == columnCount(); }
    public int rowCount() { return numbers.length; }
    public int columnCount() {return numbers[0].length; }

    private static int[][] deepCopy(final int[][] source)
    {
       // note we ignore error cases that don't apply because of 
       // invariants in the constructor:
       assert(source != null); assert(source.length != 0);
       assert(source[0] != null); assert(source[0].length != 0);
       int[][] target = new int[source.length][source[0].length];
       for (int i = 0; i < source.length; i++) 
          target[i] = Arrays.copyOf(source[i],source[i].length);
       return target;
    }

  public Matrix getTranspose()
  {

    int[][] trans = new int[columnCount()][rowCount()];

    for (int i = 0; i < rowCount(); i++)
      for (int j = 0; j < columnCount(); j++)
        trans[i][j] = getValue(j, i);
    return new Matrix(trans);
  }

  @Override
  public String toString()
  {
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < numbers.length; i++) 
    { 
      for (int j = 0; j < numbers[i].length; j++) 
        sb.append(' ').append(numbers[i][j]);
      sb.append('\n');
    }
    return sb.toString();
  }

  public static void main(String[] args)
  {
    final int[][] m1 = new int[][] { { 1, 4 }, { 5, 3 } };
    Matrix mat = new Matrix(m1);
    System.out.print(mat);
    System.out.print(mat.getTranspose());
  }
}
andersoj
+2  A: 

You didn't define a toString method for your Matrix class, so when you try to print a Matrix you see the result of the default toString method which prints the object's class and unique id.

sepp2k
+2  A: 
System.out.print(Mat);

it will call the toString method of the Matrix class.

So, if you want to print your Matrix, you will have to override toString method

@Override
public String toString() {
    // create here a String representation of your matrix
    // ie: String myString = "1 0 0 1\n0 1 1 1\n...";
    return "String representation of my matrix";
}
dwursteisen
+2  A: 

To display the Matrix class object when you can print on it you'll have to define the toString method in your class.

Another bug in the code it you are not setting the value of rows and colms. So when you do

numbers = new int[rows][colms];

in your constructor, rows and colms will always have their default value of 0. You need to fix that. And then you'll have to copy the matrix elements from the parameter array to numbers.

codaddict
as doing that //rows = numbers.length; //colms = numbers[0].length;
W M
yes, you can do that if all your rows have same number of columns.
codaddict
but in this way when i run the program it gives me the following:java.lang.NullPointerException:null.... any ideas..??
W M
@WM: Can you update above to show exactly what code is giving you NPE?
andersoj
@WM, as pointed out in my answer, you still have a bug in your constructor -- you need `this.numbers = ...` or you're just setting the argument `numbers` to something. The NPE probably comes from this.
andersoj
@WM: You're also coping with some weirdness -- your `getTranspose()` method is referring directly to the static `numbers` rather than the argument.
andersoj
@WM: See my answer above for a fully-working, oo'ified version
andersoj
+4  A: 

for a quick and dirty method:

public String toString() {
    return Arrays.deepToString(numbers);
}

On an unrelated note, the variables rows, colms, numbers and the methods isSquareMatrix should not be declared as static. Otherwise, when you get a transpose, you're going to end up with two matrix objects writing to the same class variables.

RD
+1: Never used `deepToString()` before, but now it's tucked neatly away in the toolbox, thanks!
andersoj
Almost every day I see something on SO that I had not realized was hidden away in the voluminous java standard library
RD