Saturday, December 25, 2010

Cleaner Implementation of CheckerboardWithCheckers

There was an old problem in the textbook that drew a black-and-white checkerboard. For this problem, we had to modify it to also add checkers.

I gave it a shot at first by simply breaking up the checkerboard into three sections. For the first three rows, make a square, add a red checker. For next three rows, just make the squares. For the last three rows, make a square, add a white checker (later change to black). Needless to say, this is a functional but un-elegant bit of code that re-creates all my loops three times, and if I ever needed to edit the conditions for laying down squares or checkers, I’d easily introduce bugs.

This new version is cleaned up a bit.

First, we put down the checkerboard. For each integration of the loop, we make a square in the proper spot. If it’s an “every other” square, both horizontally and vertically, make it filled. Then add the square to the board.

Next we do the checkers. Make a new GOval for every iteration of the loop; we won’t necessarily plop it down on the board, only if that spot is a proper place for a checker. (Programming experts, is this a bad practice?) If we’re on an iteration of the loop that’s for an “every other” square (we can reuse the code for “sq.setFilled” here) AND if it’s in one of the first three rows, make it red and add it to the board. (Repeat for the last three rows, except make the checker black instead of red.)

Here’s what the code looks like:

/*
* File: CheckerboardWithCheckers.java
* Name: Chu
* Section Leader: Geoff clearly, because without him there would be no acm.jar 
* in this Eclipse workspace, and then where would we be?
* ------------------
*/

import acm.graphics.*;
import acm.program.*;
import java.awt.*;

public class CheckerboardWithCheckers extends GraphicsProgram {

    //private constants
    private static final int N_ROWS = 8;
    private static final int N_COLUMNS = 8; 

    public void run() {
        double sqSize = (double) getHeight() / N_ROWS; 
        double boardWidth = N_COLUMNS * sqSize; 

        for (int i = 0; i < N_ROWS; i++) { //for the outer rows
            for (int j = 0; j < N_COLUMNS; j++) { //For columns within each row
    
                //First make the checkerboard:
                double x = (j * sqSize) + (getWidth()/2 - boardWidth/2);
                double y = i* sqSize;
                GRect sq = new GRect(x, y, sqSize, sqSize); //Make up a new square with the proper coordinates 
                if ((i+j) % 2 != 0) { //If it's every other square, both by row and column
                    sq.setColor(Color.BLACK);
                    sq.setFilled(true); //Make it filled
                    sq.setFillColor(Color.GRAY);   
                }
                add(sq); 
    
                //Now for the checkers:
                GOval checker = new GOval (x, y, sqSize, sqSize);
                checker.setFilled(true);
                if ((i+j) % 2 != 0) {//If it's every other square... (This is bad kids! Don't repeat code.)
                    if ((i < 3)) {//AND the first three rows, make red checkers 
                        checker.setFillColor(Color.RED);
                        add(checker);
                    }
                    else if ((N_ROWS-4 < i) && (i < N_ROWS)) { //If it's every other AND the last 3 rows, make white checkers
                        checker.setFillColor(Color.BLACK); 
                        add(checker);
                    } 
  
                }
    
             }
 
         }
    }
  
} 
 
Here’s what you get when you run it:
The above code is perfectly functional, except we reuse the code that checks for “every other” square, specifically “if ((i+j) % 2 != 0).” I’d wanted to use the same “if” statement for both “sq.setFilled” and “add(checker)”. However, I don’t think I can; the “add(sq)” method applies whether it’s a filled square or not, so I have to put it outside the “if every-other” loop. The add(checker) method does happen only on the “every-other” squares, so it’s inside the loop. This means adding a square comes AFTER adding a checker, so the squares go on top of the checker and we can’t see the checkers. Example code for CheckerboardWithoutRepeatingCode:
import acm.graphics.*;
import acm.program.*;
import java.awt.*;

public class CheckerboardWithoutRepeatedCode extends GraphicsProgram {

    public void run() {
        double sqSize = (double) getHeight() / N_ROWS; 
        double boardWidth = N_COLUMNS * sqSize; 

        for (int i = 0; i < N_ROWS; i++) { //for the outer rows
            for (int j = 0; j < N_COLUMNS; j++) { //for the inner rows
                double x = (j * sqSize) + (getWidth()/2 - boardWidth/2);
                double y = i* sqSize;
                GRect sq = new GRect(x, y, sqSize, sqSize); //Make up a new square with the proper coordinates
   
                if ((i+j) % 2 != 0) { //If it's every other square, both by row and column
                    sq.setFilled(true); //Make it filled
          
                    //Now for the checkers:
                    GOval checker = new GOval (x, y, sqSize, sqSize);
                    checker.setFilled(true);
     
                    if ((i < 3)) {//If it's every other AND the first three rows, make red checkers 
                        checker.setFillColor(Color.RED);
                        add(checker);
                        }
                    else if ((N_ROWS-4 < i) && (i < N_ROWS)) { //If it's every other AND the last 3 rows, make white checkers
                        checker.setFillColor(Color.WHITE); 
                        add(checker);
                    } 
  
                    }
    
                add(sq); //Plop that square down-- DAMN! So perfect except the squares go on top of the checkers...
                         //We have to make the add(sq) come before the add(checker)  
                }
 
        }
    }
 
    //private constants
    private static final int N_ROWS = 8;
    private static final int N_COLUMNS = 8; 
}
What it looks like:

Does anyone have ideas on how to condense this code and possibly not repeat the check for “if every-other” square?

5 comments:

  1. Good stuff, and you're getting really good at recognizing what needs to happen to write leaner/cleaner code! At first glance, one quick way to get the order of adding stuff to the board might be to set the piece to null first, and then after the square is added, only add the piece to the board if it's not null. E.g.:

    // ...

    // as ~line 16 of CheckerboardWithoutRepeatingCode
    piece = null

    if (playable square) {
      fill the square
      if (red player start row) {
        piece = new piece
        fill the piece with red
      } else if (white player start row) {
        piece = new piece
        fill the piece with white
      }
    }

    add square to board
    if (piece not null) {
      add piece to the board
    }

    // ...

    Also, line 28 doesn't really need the "&& (i < N_ROWS)" since the outer for loop already make sure that will always be true.

    ReplyDelete
  2. Another (even nicer) way, without using null:

    // @line 17 of CheckerboardWithoutRepeatingCode
    if (playable square) {
      fill the square
      add the square to the board
      if (red player start row) {
        piece = new piece
        fill the piece with red
        add the piece to board
      } else if (white player start row) {
        piece = new piece
        fill the piece with white
        add the piece to board
      }
    } else {
      add the square to board
    }

    // ...

    ReplyDelete
  3. That's good! but....then you use the "piece = new piece" code twice....that's no better than using the "if (playable square)" code twice, right?

    ReplyDelete
  4. True, the code is there twice now, but the best way to reuse it is to pull it out into a method. Basically you want a new method, outside of run(), that takes a color and adds the appropriate piece to the board, i.e.:

    void addPiece(color) {
      piece = new piece
      fill the piece with color
      add the piece to board
    }

    Then the code of my previous comment becomes:

    // ...
      if (red player start row) {
        addPiece(red)
      } else if (white player start row) {
        addPiece(white)
      }
    // ...

    Try it out and lemme know if that works!

    ReplyDelete
  5. ooh, I like that strategy. I wouldn't have been able to do it last week, but I just finished reading the chapter on private methods and am working on my first where the type is a graphic, so this will also be good practice.

    ReplyDelete