patternjavaMinor
Creating a chess board
Viewed 0 times
creatingchessboard
Problem
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:
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?
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?
Solution
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
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
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
the number 64 is
Formatting
Instead of this:
The convention is Java is this:
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:
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;
}
}Code Snippets
private void updateTileColor(int tileNumber)
{
if((tileNumber % 8) != 0)
{
tileColor = (tileColor == Color.WHITE) ? Color.BLACK : Color.WHITE;
}
}private void updateTileColor(int tileNumber) {
if ((tileNumber % 8) != 0) {
tileColor = (tileColor == Color.WHITE) ? Color.BLACK : Color.WHITE;
}
}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;
}
}Context
StackExchange Code Review Q#120653, answer score: 3
Revisions (0)
No revisions yet.