3
\$\begingroup\$

Search in a 2D array to check if there is a column or a row in the 2D Array thats contains all the word “error”, if yes return the index of that row or column else return 0;

Exception: if the row index equals the column index then it does not matter if contains the word "error" or not. FYI, n is the size of 2D array (number of rows = number of columns)

I wrote this code and I want to check with you if you agree with the logic:

int search(int n, String[][] myArray) {
        int j, k;
        boolean isError;

        //To search per ROW
        for (int i = 1; i <= n; i++) {
            j = 1;
            isError = true;

            while (j <= n && isError) {
                if ((i != j) && (myArray[i][j] != "error")) {
                    isError = false;
                }

                if (j == n && isError) {
                    return i;
                } else {
                    j++;
                }
            }
        }//end for loop

        //To search per COLUMN
        for (int i = 1; i <= n; i++) {
            k = 1;
            isError = true;

            while (k <= n && isError) {
                if ((i != k) && (myArray[k][i] != "error")) {
                    isError = false;
                }

                if (k == n && isError) {
                    return i;
                } else {
                    k++;
                }
            }
        }//end for loop

        return 0;

    }
\$\endgroup\$
2
\$\begingroup\$

My Java is a bit rusty but I'm not seeing any logical issues with the code doing what you want it to.

Since you return just the row or column index, the caller can't tell from that if the match was a row or a column.

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview 1201ProgramAlarm. Hope you enjoy the site. \$\endgroup\$ – Legato Sep 30 '15 at 3:33
1
\$\begingroup\$

There are several issues with this code.

The biggest one is that you use != to check for string equality instead of !String.equals().

The parameter n shouldn't need to be specified, since it can be inferred from the dimensions of myArray. As others have noted, the function is weird in that it ignores the first row and the first column, that it uses 0 as a special indicator that no match was found, and that the return value doesn't tell you whether it was a row or column that was found.

I'm not a fan of variable names i, j, and k. Variable names like row and col would have been clearer. The flag variable isError can be eliminated if you just structure the inner loops properly. (In general, flag variables are a poor way to direct flow of control; keywords like break, continue, and return are preferable.)

There is no sense in putting a test like if (j == n …) inside the loop, since it is only relevant when the loop terminates.

I'd write something more like this:

for (int row = 0; row < myArray.length; row++) {
    int col;
    for (col = myArray[row].length - 1; col >= 0; col--) {
        if (row != col && !"error".equals(myArray[row][col])) {
            break;
        }
    }
    if (col < 0) {
        return row;  // Every relevant column in this row is "error"
    }
}
…
return …;  // Some value to indicate that no match was found
\$\endgroup\$
  • \$\begingroup\$ j == n should be inside the loop (while j <= n) \$\endgroup\$ – Mike Sep 30 '15 at 21:26
1
\$\begingroup\$
return 0;

0 is a valid array index... I will strongly suggest going with -1. Alternatively, throw a suitable Exception type only if this is indeed an exceptional condition.

edit: Actually, I'm not sure if this is a bug or intentional, but indices of array length \$n\$ are in the range \$[0, n-1]\$ (i.e. \$0\$ to \$n-1\$ inclusive), not \$[1, n]\$. If n is indeed passed in as the array length, then you will encounter an ArrayIndexOutOfBoundsException. Assuming that is handled properly, then take note that your current solution will skip the first row and column as the starting index is 1.

Also, n can be derived from myArray.length, so you may not need it as a method argument unless callers can specify an arbitrary 'stop index' (needs to be checked against myArray.length too).

There is a slightly easier way for comparing rows, using Arrays.deepEquals(Object[], Object[]):

for (int i = 0; i < myArray.length; i++) {
    String[] errors = Collections.nCopies(myArray.length, "error").toArray(new String[0]);
    errors[i] = myArray[i][i];
    if (Arrays.deepEquals(errors, myArray[i])) {
        return i;
    }
}

One more thing about comments: I think your comments are redundant, especially the //end for loop. Comments should document the why, not the how, so there is no need for them to document something that is already obvious code-wise.

\$\endgroup\$
  • \$\begingroup\$ Your row comparison does not work for OP because OP does not want myArray[i][i] to be checked for "error". \$\endgroup\$ – 1201ProgramAlarm Sep 30 '15 at 5:01
  • \$\begingroup\$ @1201ProgramAlarm thanks for pointing it out, updated my answer. \$\endgroup\$ – h.j.k. Sep 30 '15 at 6:15

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.