4
\$\begingroup\$

I am solving a coding challenge whose objective is to print the right diagonal of a matrix. And below is my code to do that.

package basics;
import java.util.*;
/**
Problem statement : Print right diagonal of a matrix. Example: For input : a b, c d ; the right diagonal is b c
**/
public class PrintRightDiagonal{

    private int rowSize;
    private int colSize;
    private char[][] matrix;
    private Scanner scanner = new Scanner(System.in);
    private String[] rows;
    private String userInput;

    public static void main(String[] args){
        PrintRightDiagonal printRightDiagonal = new PrintRightDiagonal();
        printRightDiagonal.readMatrixDimensions();
        printRightDiagonal.readInputEntries();
        printRightDiagonal.validateInputEntries();
        printRightDiagonal.insertIntoArr();
        printRightDiagonal.findRDiagonal();
    }   

    private void readMatrixDimensions(){
        System.out.println("Please enter the rowsize of the 2D array");
        rowSize = scanner.nextInt();
        System.out.println("Please enter the colsize of the 2D array");
        colSize = scanner.nextInt();
        if(rowSize!=colSize){
            System.err.println("Cannot find the diagonal of a matrix because the rowsize and column size are different");
            System.exit(0);
        }
    }

    private void readInputEntries(){
        System.out.println("Please enter the rows separated by comma. Example if the matrix is a 2x2 enter i/p as ab,cd");
        scanner.nextLine();
        userInput = scanner.nextLine();
    }

    private void validateInputEntries(){
        rows = userInput.split(",");
        if(rows.length!=rowSize){
            System.err.println("Entered rows doesn't match with the rowSize");
            System.exit(0);
        }
        for(String row : rows){
            if(row.length()!=colSize){
                System.err.println("Entered cols doesn't match with the colsize");
                System.exit(0);
            }
        }
    }

    private void insertIntoArr(){
        //Initialize matrix
        matrix = new char[rowSize][colSize];
        int index=0;
        for(String row : rows){
            matrix[index++] = row.toCharArray();
        }
        //print matrix once
        System.out.println("Here is your matrix.");
        for(int i=0; i<rowSize; i++){
                for(int j=0; j<colSize; j++){
                    System.out.print(matrix[i][j] +" ");
                }
                System.out.println("");
        }
    }

    private void findRDiagonal(){
        System.out.println("Here is the Right diagonal of the matrix.");
        for(int i=0,j=colSize-1; i<rowSize && j>=0; i++,j--){
                System.out.print(matrix[i][j] + " ");
        }
    }

}

The above works fine. But, I doubt if the code is readable, is it following an OOP way. I think the class name sounds like a method name. Given a code challenge like this how to ensure that we don't miss the basics like class naming, methods organization. In the above code I am not sure if the method organization is correct. Consider the following pseudo code.

//pseudo code

    class Name{
     main(){
      a();
      b();
      c();
     }
    }

As in the above pseudo code, the method a(), b(), c() should be written above the main() method or below the main()?

And here is my thought to improve this code. Write a class which just has a main method and from it initialize another class and call its methods, so that main class duty is just to initialize the worker class and call its operations. But, do we really need 2 classes in this case?

Throw some light on this.

\$\endgroup\$
6
\$\begingroup\$

Sequential cohesion

From Code Complete, chapter 7, High-quality routines:

Sequential cohesion exists when a routine contains operations that must be performed in a specific order, that share data from step to step, and that don’t make up a complete function when done together.

Consider these lines in the code:

printRightDiagonal.readMatrixDimensions();
printRightDiagonal.readInputEntries();
printRightDiagonal.validateInputEntries();
printRightDiagonal.insertIntoArr();
printRightDiagonal.findRDiagonal();

If you reorder these calls, the program will compile, but it won't work correctly. This is undesirable.

Consider this alternative:

Matrix matrix = readMatrix(new Scanner(System.in));
matrix.printRightDiagonal();

Where readMatrix does:

int size = readMatrixSize(scanner);
char[][] data = readMatrixEntries(scanner, size);
return new Matrix(data);

You can guess what readMatrixSize and readMatrixEntries do. The point is, each function does one thing (functional cohesion), the relationship between statements is clear, and reordering the statements will result in compile errors, protecting you from runtime errors.

Encapsulation

The idea of encapsulation is limiting the scope of low-level details (information hiding), to reduce the mental burden of understanding a program.

All these fields in the class are a mental burden:

private int rowSize;
private int colSize;
private char[][] matrix;
private Scanner scanner = new Scanner(System.in);
private String[] rows;
private String userInput;

They are a mental burden because these variables are accessible in the entire class, so if you want to know where they are read or modified, you have to read the entire program.

The fields rows and userInput are the worst offenders, because they are only used during input. They should not be exposed like this to the entire class, but encapsulated by the input mechanism, hidden from the rest of the program.

Side effects, semantic rules, failure of encapsulation

This method suggests a deep flaw in the program:

private void readInputEntries(){
    System.out.println("Please enter the rows separated by comma. Example if the matrix is a 2x2 enter i/p as ab,cd");
    scanner.nextLine();
    userInput = scanner.nextLine();
}

What is the scanner.nextLine(); doing there? It's there because in the earlier readMatrixDimensions call, after reading the matrix dimensions, the end of line character is not read. This is a big problem, for several reasons:

  1. Huge burden on the reader. In order to understand the purpose of this line, you have to read the caller of this method, and the calls before this method that affected scanner.

  2. readMatrixDimensions is clearly not doing its job. It leaves the scanner in a state that's not ready for the next use. It forces the rest of the program to understand and remember the low-level details of its implementation.

  3. If you remove this line, the method will look correct, perfectly fine, even though the program will be broken.

The fix is relatively simple. Make sure that methods don't have such invisible rules, hidden dependencies on the implementation details outside of them. In this example, if you move scanner.nextLine() to readMatrixDimensions, this problem disappears.

Usability, logic

There's a mix of usability and logical issues here:

System.out.println("Please enter the rowsize of the 2D array");
rowSize = scanner.nextInt();
System.out.println("Please enter the colsize of the 2D array");
colSize = scanner.nextInt();
if(rowSize!=colSize){
    System.err.println("Cannot find the diagonal of a matrix because the rowsize and column size are different");
    System.exit(0);
}

The messages talk about a 2D array, but that's an implementation detail. The data of a matrix could be stored in a 1D array. Users don't have to know such low-level implementation details.

The program is designed to work with a square matrix. So why give the user the opportunity to enter different row and column sizes? Ask to input a single size value. That will effectively prevent the user from entering invalid values.

The validation is insufficient. 0 or negative values will not work out well.

Exiting the program in case of invalid input is not very user friendly. Consider asking until there is a valid value, for example:

private int readMatrixSize(Scanner scanner) {
    System.out.println("Please enter the size of the matrix");
    int size;
    while (true) {
        size = scanner.nextInt();
        if (size > 0) {
            break;
        }
        System.out.println("The size must be positive. Try again!");
    }
    // read the end of line character
    scanner.nextLine();
    return size;
}

Single responsibility principle

insertIntoArr does two things:

  • Creates a matrix
  • Prints the matrix

These should be separated into createMatrix and printMatrix methods.

Naming

findRDiagonal prints the diagonal. There's not much there to "find". So printRightDiagonal would be a better name.

Program control

It's generally a bad practice to call System.exit in methods other than main. This kind of control is best in the main method, there should be no other exit points. If at any point an invalid input occurs, it would be better to throw a custom exception, and let the main method handle it.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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