2
\$\begingroup\$

I am required to write a simple matrix program and swap the diagonal.

Given the input

1 2 3 
4 5 6
7 8 9

After swapping the diagonal it should be

3 2 1
4 5 6
9 8 7

Here is my code:

public static void main(String[] args) {
    int i,j = 0,m,k;
    int Number[][] = new int [3][3];
    Scanner scanner = new Scanner(System.in);
    System.out.println("Enter the elements of matrix");
    for(i=0;i<=2;i++)
    {
        for(j=0;j<=2;j++)
        {
            m = scanner.nextInt();
            Number[i][j] = m;
        }
    }
    System.out.println("This is the original matrix");
    for(i=0;i<=2;i++)
    {
        for(j=0;j<=2;j++)
        {
            System.out.print(Number[i][j]+"\t");
        }
        System.out.println();
    }
    //Swapping
    for(i=0;i<=3;i++)
    {
        k=Number[i][j];
        Number[i][j] = Number[i][3-i-1];
        Number[i][3-i-1] = k;
    }
    for(i=0;i<=2;i++)
    {
        for(j=0;j<=2;j++)
        {
            System.out.print(Number[i][j]+"\t");
        }
        System.out.println();
    }
}
\$\endgroup\$

closed as off-topic by 200_success Nov 2 '16 at 20:22

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ you requested output does not look like being swapped diagonal. shouldn't it be 147/258/369 instead? \$\endgroup\$ – Timothy Truckle Nov 2 '16 at 11:03
  • \$\begingroup\$ i dont know but my lecturer requested that. I have search through internet and there's not such example \$\endgroup\$ – Ardley Nov 2 '16 at 14:51
  • \$\begingroup\$ If your requested output i right, then you only have to exchange the 4 values in the corners of the matrix. No loop needed... \$\endgroup\$ – Timothy Truckle Nov 2 '16 at 18:44
0
\$\begingroup\$

I assume that your code produces the desired output, so I'll comment on the code style only here.

  1. While doing everything in the main method may work for small programs, it's not a good idea in general. Ideally, one method should do one thing. Moreover, exactly the same code that prints the matrix is used twice in your solution. It's a very good reason to create a separate method for it. In general, I would split your program into at least three separate methods: one of them would read the matrix, the other one would perform a required transformation and the last one would print the matrix to the standard output. It looks roughly like this:

    int[][] readMatrix() {
        ...
    }
    
    void swapDiagonal(int[][] matrix) {
        ...
    }
    
    void printMatrix(int[][] matrix) {
        ...
    }
    
  2. The comments inside the method (like //Swapping) are an indicator that a piece of code this comments corresponds to should be probably factored out to a separate method (of course, it's not always the case. It is the case here, though). Once or code calls the swapDiagonal method, there is no need for such comments anymore. The code becomes self-documented, which is a great thing as comments can become obsolete if you change the code in the future. There's no way to ensure that the piece of code followed by the //Swapping comment actually swaps anything after changes are made (again, it's not a big deal for a such a small program, but it can become a serious issue if you implement a larger program using the same practices).

  3. You can get rid of magic constants (like 3 and 2) in your code by creating a constant variable for them (for instance, you can set final static int MATRIX_SIZE = 3 and use it everywhere in your code. It communicates the intent more clearly and makes possible future changes easier (you need to change just one constant instead of looking for all 3's and 2's around your code).

  4. Keeping the scope of each variable as small as possible is a good practice. The declaration of loop counters outside the loop is not just unnecessary, it's also sort of misleading (as it's normally done only when the value of the counter is used outside the loop, which is not the case here). That is, the only variable you need to declare in the begging of the method is the array to hold the matrix. The loops are usually implemented this way:

    for (int i = 0; i < MATRIX_SIZE; i++) {
        for (int j = 0; j < MATRIX_SIZE; j++) {
            // do something with the (i, j) element here
        }
    }  
    
  5. Variable naming. It's conventional for the names of variables to start with a lowercase letter. Moreover, a Number doesn't look like a good name for a matrix to me. It's confusing. You could call matrix, for instance.

  6. Code formatting. It's conventional to surround binary operators with whitespaces. The opening curling bracket shouldn't go to the next line according to the Java coding conventions. It is also common to use int[][] array, not C-style int array[][] in Java (it has the same semantics, though).

  7. You could also get rid of redundant variables and assignments (I'm not talking about performance, I' talking about communicating your intent as clearly as possible):

    m = scanner.nextInt();
    Number[i][j] = m;
    

    looks quite strange. Why not use:

    Number[i][j] = scanner.nextInt();
    

    instead?

\$\endgroup\$

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