2
\$\begingroup\$

For a simple project, I represent a two-dimensional grid as a one-dimensional array, with both row and column indices between 1 and gridHeight. When retrieving a cell from the grid, I first need to check if both row and column IDs are within allowed bounds.

public void checkIfAllowed(int row_or_column) {
    if ((row_or_column < 1) || (row_or_column > gridHeight)) {
        throw new IndexOutOfBoundsException("Row or column ID is not acceptable.");
    }
}

public boolean getCellById(int row, int column) {
    checkIfAllowed(row);
    checkIfAllowed(column);
    return grid[row * gridHeight + column];
}

Can it possibly be improved?

There are two things about this code that I don't particularly like:

  • Somewhat repetitive code in getCellById.
  • It asks for permission instead of forgiveness: first checks everything, and then does the action. I would prefer a try .. catch construction (something like "try to return a value and then catch IndexOutOfBoundsException), but would it really work for Java? The array dimension is defined during run time, and initial declaration for my grid is just private boolean[] grid;.
\$\endgroup\$
3
  • 1
    \$\begingroup\$ throw out all your checkIfAllowed stuff - you'll still get IndexOutOfBoundsException, though without message (which is not informative anyway) \$\endgroup\$ – Alexei Kaigorodov Sep 9 '13 at 8:45
  • \$\begingroup\$ You do in fact need the check as without you could, for example on a 25x25 grid, access (4,30). This would result in index (4*25)+30 = 130, which is a valid index, but invalid grid coordinate. I posed a way to overcome this below. \$\endgroup\$ – aglassman Sep 11 '13 at 17:02
  • \$\begingroup\$ Why not use a two dimensional array and let Java take care of everything? \$\endgroup\$ – raptortech97 Sep 27 '14 at 13:38
2
\$\begingroup\$

A quick solution would be to use absolute value and mod on the input by the grid size. That way you could put in any numbers and never get an Out Of Bounds exception.

grid[Math.abs(row)%gridHeight*gridHeight + Math.abs(column)%gridHeight]
\$\endgroup\$
2
\$\begingroup\$

Not sure if I've understood yout question correctly, but I assume, that your grid is a square grid (you test both, maximal high and width, as a lower than grid's height).

Referring to your first question: There are many options. You can make a method which takes two parameters at a time:

public void checkIfAllowed(int row, int column){
      boolean areCorrect = checkIfRowAllowed(row) && checkIfColAllowed(column);
      if(!areCorrect){
           throw new IndexOutOfBoundsException("Row or column ID is not acceptable.");
      }
}

private boolean checkIfAllowed(int row_or_column) {
    return (row_or_column < 1 || row_or_column > gridHeight);
} 

Now, a client is not forced to check each row and column in separate lines. Just an example:

public boolean getCellById(int row, int column) {
      try{    
          checkIfAllowed(row, column);
          return grid[row * gridHeight + column];

      } catch (IndexOutOfBoundsException e) {
           // log an error, or do something else
      }
}

But this exception handling seems strage for me. I would simply throw it in getCellById(int row, int col) and handle when this method is invoked.

\$\endgroup\$
2
  • \$\begingroup\$ I would edit the IndexOutOfBoundsException message with IndexOutOfBoundsException("Row (" + row + ") or column (" + column + ") ID is not acceptable."); Without say nothing it's like "Hey something is wrong and i know what, but i will not tell you anything put a breakpoint here if you want to know." \$\endgroup\$ – Marco Acierno Sep 26 '14 at 20:37
  • \$\begingroup\$ @MarcoAcierno Yeah, you are certainly right... By "// log an error, or do something else" I meant exactly what you've said - some logging message in particular. \$\endgroup\$ – guitar_freak Sep 27 '14 at 10:32
0
\$\begingroup\$

I cannot see anything wrong, both with checking the arguments first and the little bit of repetitive code. However, there are two things that I think need fixing:

  • The exception message is not helpful. It says "row or column" and neither mentions bounds nor actual value. That does not make debugging easy. Once you fix that, there will be no repetitive code anymore.

  • More importantly: Your code seems to have a bug. You are disallowing row and column indexes of 0. That does not go well with the fact that your data is stored in grid which is an array. That effectively means the entire first row (gridHeight many elements in the array) are unused, but still need to be allocated. If you create the grid as new boolean[gridHeight * gridHeight] this would still lead to an ArrayIndexOutOfBoundsException although it is not detected by checkIfAllowed(). Rather subtract one from the indices and save on the array length.

Finally, avoid underscores in Java identifiers, or at least don't mix them with camel case.

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