3
\$\begingroup\$

I made a method that takes an array and converts it to a matrix, given that it is a valid conversion. However, there is some redundant code in the beginning and I thought maybe you could help me? I am new to programming so avoid complicated imports and such.

    private int[][] toMatrix(int[] arr) {

    if(validConversion(arr)){
        int index = 0;
        //redundant
        int row = (int) Math.sqrt(arr.length);
        int col = (int) Math.sqrt(arr.length);

        int[][] matrix = new int[row][col];
        for(int i = 0; i<row; i++){
            for(int j = 0; j<col; j++){
                matrix[i][j] = arr[index];
                index++;
            }
        }
        return matrix;
    }
    return null;
}


boolean validConversion(int[] arr) {
    return arr.length == 1 || arr.length % Math.sqrt(arr.length) == 0;
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ The method name should really be called toSquareMatrix() \$\endgroup\$ – Ted Brownlow Jan 18 at 18:18
5
\$\begingroup\$
private int[][] toMatrix(int[] arr) {

Don't shorten names just because you can.


if(validConversion(arr)){

This does not need to be an extra function, actually, especially if you drop the "1" special case. An array of one item can technically be converted to a matrix with one item...it's just not much of a matrix.

int matrixSize = (int) Math.sqrt(array.length);

if ((array.length % matrizSize) == 0) {

You can also reuse matrixSize now.


        for(int i = 0; i<row; i++){
            for(int j = 0; j<col; j++){

I'm a very persistent advocate for that you're only allowed to use single-letter variable names when dealing with dimensions. In this case, using column and row would make the code very nicely readable:

for (int row = 0; row < matrixSize; row++) {
    for (int column = 0; column < matrixSize; column++) {
        matrix[row][column] = array[index];
        index++;
    }
}

index++ could be rolled into the assignment:

matrix[row][column] = array[index++];

I'm torn whether that actually improves readability or not, though.


boolean validConversion(int[] arr) {

Members and functions without an explicit visibility modifier are package-private. In my opinion, package-private should be avoided as it easily can lead to unhealthy couplings between classes in the same package.


Consider whether you want to perform additional error checks and throw IllegalArgumentExceptions if they don't add up, for example when the given array is null and so on.

\$\endgroup\$
3
  • \$\begingroup\$ An alternative rule (which I follow) for single-character names is that the larger the scope, the more descriptive the name. Loop index variables that span 2 or 3 lines ought to be very short, probably a single character. Whereas the function parameter has much greater scope and should be longer. \$\endgroup\$ – Toby Speight Jan 27 at 7:57
  • \$\begingroup\$ @TobySpeight My point with that rule is, that every line should be readable on its own, without having to go hunting for definitions (too much). As an example, performCalculation(i, j) and performCalculation(groupIndex, rowIndex). \$\endgroup\$ – Bobby Jan 27 at 16:01
  • \$\begingroup\$ I wasn't disagreeing - just a different means to similar ends. I don't go so far as "a single line on its own", but a small group of lines (e.g. such as this for loop) should be intelligible. Readers can choose what's right for them. Great answer, by the way! \$\endgroup\$ – Toby Speight Jan 28 at 7:51
3
\$\begingroup\$

We could evaluate the square root just once:

    int width = (int)Math.sqrt(arr.length);
    if (width * width != arr.length) {
        // not square - invalid
        return null;
    }

    int height = width  // not really needed, since we know matrix is square

It's not clear why you reject arrays of 0 or 1 elements, so I removed that test.

We should consider throwing an exception (perhaps InvalidArgumentException?) rather than returning a null object when the test fails.

\$\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.