7
\$\begingroup\$

One of my assignments was to create a chessboard. In this assignment I could not use an array, or a form of list or what not. Below is my code for the creation of the chessboard:

import java.awt.Graphics2D;
import java.awt.Rectangle;
import java.awt.Color;

public class Checkerboard
{
    private int xPositions;
    private int yPositions;
    private int squareSize;

    private Color tileColor;

    public Checkerboard()
    {
        xPositions = 0;
        yPositions = 0;
        squareSize = 50;

        tileColor = Color.WHITE;
    }

    public void draw(Graphics2D g2)
    {
        for(int i = 1; i <= 64; i++)
        {
            Rectangle tile = new Rectangle(xPositions, yPositions, squareSize, squareSize);

            g2.setColor(tileColor);
            g2.fill(tile);

            updateTileColor(i);
            updatePositions(i);
        }
    }

    private void updatePositions(int tileNumber)
    {
        if((tileNumber % 8) == 0)
        {
            xPositions = 0;
            yPositions = yPositions + squareSize;
        }
        else
        {
            xPositions = xPositions + squareSize;
        }
    }

    private void updateTileColor(int tileNumber)
    {
        if((tileNumber % 8) != 0)
        {
            tileColor = (tileColor == Color.WHITE) ? Color.BLACK : Color.WHITE;
        }
    }
}

Could anyone tell me as to whether this code is optimal for the assignment, if any bad practices have been made, or any way I can generally improve my code and my knowledge of the code?

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review community. Congratulations for asking a good question! \$\endgroup\$ – Alexei Feb 20 '16 at 22:28
3
\$\begingroup\$

Avoid mutable state

Mutable state, in this example the modifying of the x and y positions to draw, causes all kinds of troubles.

For example, if you call the draw method a second time, it will paint another board right after the first one. This might be surprising to users. Normally we expect that calling a method multiple times has the same effect, which is often not the case with methods that mutate state.

Another common problem with mutable state is that it breaks thread-safety. If draw gets called from multiple threads in parallel, it will most certainly paint rubbish.

Granted, you probably don't plan to call this method multiple times and from multiple threads. But as a general good practice, it's good to avoid mutable state when possible.

Using constants

Avoid hard-coded (magic) numbers. The number 8 is the SIZE of the board, the number 64 is SIZE * SIZE. If you refer to them that way, the intention of the code becomes all the more clear. And if you decide to paint a larger (or smaller) board, you can change one value in one place.

Formatting

Instead of this:

private void updateTileColor(int tileNumber)
{
    if((tileNumber % 8) != 0)
    {
        tileColor = (tileColor == Color.WHITE) ? Color.BLACK : Color.WHITE;
    }
}

The convention is Java is this:

private void updateTileColor(int tileNumber) {
    if ((tileNumber % 8) != 0) {
        tileColor = (tileColor == Color.WHITE) ? Color.BLACK : Color.WHITE;
    }
}

If you use an IDE like IntelliJ or Eclipse, the auto-reformat feature will format the code for you as per the convention.

Alternative implementation

Others have given you various alternatives, here's mine, without mutating state, and applying the suggestions above:

public class Checkerboard {

    private static final int SIZE = 8;

    private final int squareSize = 50;

    public void draw(Graphics2D g2) {
        for (int pos = 0; pos < SIZE * SIZE; pos++) {
            int x = calculateX(pos);
            int y = calculateY(pos);
            Color color = calculateColor(pos);

            Rectangle tile = new Rectangle(x, y, squareSize, squareSize);

            g2.setColor(color);
            g2.fill(tile);
        }
    }

    private int calculateX(int pos) {
        return (pos % 8) * squareSize;
    }

    private int calculateY(int pos) {
        return (pos / 8) * squareSize;
    }

    private Color calculateColor(int pos) {
        int offset = (pos % 16) < 8 ? 0 : 1;
        return (pos + offset) % 2 == 0 ? Color.WHITE : Color.BLACK;
    }
}
\$\endgroup\$
2
\$\begingroup\$

Just what pops in my mind seeing the code:

1. Variable names

xPositions should be xPosition or simply an x (small context allow simple variable names) the same for yPositions

2. Constants

Some things are constants so they can be declared at class level:

// maybe this should be provided as a parameter, though
private final int squareSize = 50;
private final int BoardMatrixSize = 64;

3. Colors can be declared in a separate array

Although, chess classic colors are black and white, this does not mean that you will always display them as pure Black & White, so it would make sense to define them in a place to be easily changed (maybe configurable in the future):

public static Color colors[] = new Color[] {Color.White, Color.Black};

4. Simpler and more natural iteration

Your logic is fine, however a chess board is 8 x 8 and a natural way to iterate is by rows and columns, rather than tile by tile.

final int BoardSize = 8;

public void draw(Graphics2D g2)
{
    int xPos = 0;
    int yPos = 0;
    int colorIndex = 0;

    for (int y = 0; y < BoardSize; y ++)
    {
        for (int x = 0; x < BoardSize; x ++)
        {
            Color tileColor = colors[colorIndex];
            Rectangle tile = new Rectangle(xPos, yPos, squareSize, squareSize);
            g2.setColor(tileColor);
            g2.fill(tile);

            colorIndex = (colorIndex + 1) % 2;
            xPos += squareSize;
        }

        yPos += squareSize;
        xPos = 0; 
    }
}

5. Contracted assignments. E.g.

yPositions = yPositions + squareSize; can be yPositions += squareSize

\$\endgroup\$
  • 2
    \$\begingroup\$ One of the restrictions of the assignment was no arrays. \$\endgroup\$ – 200_success Feb 20 '16 at 22:51
  • 1
    \$\begingroup\$ You have x and y mixed up in your loop headers. \$\endgroup\$ – 200_success Feb 20 '16 at 22:54
  • 1
    \$\begingroup\$ @200_success - I have fixed the x and y loops. Damn, I have missed the restriction. However, it is a strange requirement - usually, it happens that programmers mix data and logic when they can define array, maps or similar structures to separate data from the logic. I would emphasize learning towards this aspect. Thanks for pointing out the issues. \$\endgroup\$ – Alexei Feb 21 '16 at 7:53
2
\$\begingroup\$

Pattern

****
****
****
    ****
    ****
    ****
****
****
****
    ****
    ****
    ****

...

This pattern repeats every two tiles horizontally. So you just need to do the same for half the amount of tiles. Also you can paint the whole background in one color and then just paint one kind of tile.

Constants:

Set the amount of tiles as a constant.

Immutability:

You can make this whole class immutable. (Unless you will need to change it's state for some reason in the future).

Constructors:

You should add some overloads so the client can pick the color and coordinates.

Code:

public final class Checkerboard {

    private static final int BOARD_TILES = 8;

    private final int x;
    private final int y;
    private final int squareSize;

    private final Color tileColor1;
    private final Color tileColor2;

    public Checkerboard(int x, int y, Color tile1, Color tile2, int tileSize) {
        this.x = x;
        this.y = y;
        squareSize = tileSize;
        tileColor1 = tile1;
        tileColor2 = tile2;
    }

    public Checkerboard() {
        this(0, 0, 50);
    }

    public Checkerboard(int x, int y, int tileSize) {
        this(x, y, Color.WHITE, Color.BLACK, tileSize);
    }

    public Checkerboard(int tileSize) {
        this(0, 0, tileSize);
    }

    public void draw(Graphics2D g2) {
        int lx = x;
        int ly = y;

        paintTile(g2, tileColor1, lx, ly, BOARD_TILES * squareSize);
        int numPattern = BOARD_TILES / 2;
        int xDisplacement = 2 * squareSize;
        for (int i = 0; i < numPattern; ++i) {
            for (int j = 0; j < BOARD_TILES; ++j) {
                int newX = lx + (j % 2) * squareSize;
                paintTile(g2, tileColor2, newX, ly, squareSize);
                ly += squareSize;
            }
            ly = 0;
            lx += xDisplacement;
        }
    }

    private void paintTile(Graphics2D g2, Color color, int px, int py, int size) {
        Rectangle tile = new Rectangle(px, py, size, size);
        g2.setColor(color);
        g2.fill(tile);
    }
}

Java 8

If java 8 is available you can do it like this:

    public void draw(Graphics2D g2) {
        paintTile(g2, tileColor1, x, y, BOARD_TILES * squareSize);
        int xDisplacement = 2 * squareSize;
        IntStream.range(0, BOARD_TILES / 2)
                 .map(i -> i * xDisplacement )
                 .forEach(xOffset -> paintPattern(xOffset, g2));
    }

    private void paintPattern(int xOffset, Graphics2D g2) {
        int currentX = x + xOffset;
        IntStream.range(0, BOARD_TILES)
                 .map(j -> currentX + ((j % 2) * squareSize))
                 .reduce(y, (ly, lx) -> {
                     paintTile(g2, tileColor2, lx, ly, squareSize);
                     return ly + squareSize;
                 });
    }
\$\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.