Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
  • DRY

    Code for columns and rows is identical except the board access (board[i][j] and board[j][i] respectively). Such minute difference is hard to spot, and the reviewer (or maintainer) has hard time understanding what's going on. I recommend to factor the common part out, along the lines of:

     bool rowCheck() {
     return boardCheck(rowWize);
     }
     bool columnCheck() {
     return boardCheck(columnWize);
     }
     bool boardCheck(Callable<Integer,Integer> boardAccess) {
     ....
     value = boardAccess(i, j);
     ....
     }
    

    or with lambdas instead of callable wrappers. See this discussion this discussion for details.

  • HashMap

    seems like overkill. A bool seen[size] initialized to false and

     value = board[i][j] - 1;
     if (seen[value]) {
     return false;
     } else {
     seen[value] = true;
     }
    

    achieves the same goal with much less overhead. Of course you'd need a bound check.

    The side effect of this approach is that your way may have false positives as @CaptainMan noticed in the comment.

  • DRY

    Code for columns and rows is identical except the board access (board[i][j] and board[j][i] respectively). Such minute difference is hard to spot, and the reviewer (or maintainer) has hard time understanding what's going on. I recommend to factor the common part out, along the lines of:

     bool rowCheck() {
     return boardCheck(rowWize);
     }
     bool columnCheck() {
     return boardCheck(columnWize);
     }
     bool boardCheck(Callable<Integer,Integer> boardAccess) {
     ....
     value = boardAccess(i, j);
     ....
     }
    

    or with lambdas instead of callable wrappers. See this discussion for details.

  • HashMap

    seems like overkill. A bool seen[size] initialized to false and

     value = board[i][j] - 1;
     if (seen[value]) {
     return false;
     } else {
     seen[value] = true;
     }
    

    achieves the same goal with much less overhead. Of course you'd need a bound check.

    The side effect of this approach is that your way may have false positives as @CaptainMan noticed in the comment.

  • DRY

    Code for columns and rows is identical except the board access (board[i][j] and board[j][i] respectively). Such minute difference is hard to spot, and the reviewer (or maintainer) has hard time understanding what's going on. I recommend to factor the common part out, along the lines of:

     bool rowCheck() {
     return boardCheck(rowWize);
     }
     bool columnCheck() {
     return boardCheck(columnWize);
     }
     bool boardCheck(Callable<Integer,Integer> boardAccess) {
     ....
     value = boardAccess(i, j);
     ....
     }
    

    or with lambdas instead of callable wrappers. See this discussion for details.

  • HashMap

    seems like overkill. A bool seen[size] initialized to false and

     value = board[i][j] - 1;
     if (seen[value]) {
     return false;
     } else {
     seen[value] = true;
     }
    

    achieves the same goal with much less overhead. Of course you'd need a bound check.

    The side effect of this approach is that your way may have false positives as @CaptainMan noticed in the comment.

Source Link
vnp
  • 58.6k
  • 4
  • 55
  • 144
  • DRY

    Code for columns and rows is identical except the board access (board[i][j] and board[j][i] respectively). Such minute difference is hard to spot, and the reviewer (or maintainer) has hard time understanding what's going on. I recommend to factor the common part out, along the lines of:

     bool rowCheck() {
     return boardCheck(rowWize);
     }
     bool columnCheck() {
     return boardCheck(columnWize);
     }
     bool boardCheck(Callable<Integer,Integer> boardAccess) {
     ....
     value = boardAccess(i, j);
     ....
     }
    

    or with lambdas instead of callable wrappers. See this discussion for details.

  • HashMap

    seems like overkill. A bool seen[size] initialized to false and

     value = board[i][j] - 1;
     if (seen[value]) {
     return false;
     } else {
     seen[value] = true;
     }
    

    achieves the same goal with much less overhead. Of course you'd need a bound check.

    The side effect of this approach is that your way may have false positives as @CaptainMan noticed in the comment.

lang-java

AltStyle によって変換されたページ (->オリジナル) /