Return to Answer

Commonmark migration

edited Jun 10, 2020 at 13:24

Community Bot

• 1

Inline Side-by-side Side-by-side Markdown

answered Jun 17, 2015 at 23:41

maaartinus

• 13.6k
• 1
• 35
• 74

It can't determine a winner (yet).

I'd suggest to use Area scoring, where you can let the players continue until it gets simple enough for your algorithm.

Is Main an acceptable name for a class? Or should it be Game/App or something completely different?

I always use a prefix for my "Mains" (like GoMain) in order to be able to tell them apart even where a simple name gets used (e.g., Eclipse run configurations).

package go;

That's wrong, unless there's a TLD "go" and you own it.

##Main

public static final int BORDER_SIZE = 25;

I'd move it into the class where other view constants get defined.

##GameBoard

private static final long serialVersionUID = -494530433694385328L;

You don't need it, unless you

• really want to
  • serialize GameBoard instances
  • and save them in files
  • and use them later with a different class version
• and you're determined to handle compatibility

With other words, you really do not need it (and the Eclipse warning is a non-sense and should be switched off).

public static final int SIZE = 9;

As the SIZE holds for Grid as well, I'd move it there (since the view is just an auxiliary thing).

public static final int N_OF_TILES = SIZE - 1;

The naming has been already commented on, but I'd drop this altogether as it doesn't save much.

But actually, I'd drop SIZE as a constant anyway. You'll surely want to play on bigger boards as well.

public enum State {
 BLACK, WHITE
}

Again, this IMHO belongs to Grid (as the more fundamental class).

Having two possibilities only (rather than adding EMPTY) allows you to use

private State current_player;

(which violates naming convention a lot!), but using null as the third value for stones is ugly. I might go for

private boolean isBlacksTurn;

which is ugly, too.

---

private Grid grid;

This should be final.

private Point lastMove;

Point is an ugly mutable class which I wouldn't use anywhere except where required.

public GameBoard() {
 grid = new Grid(SIZE);

I'd use (manual) DI here, simply by passing a grid to the constructor. Again, the view is not the important part. Creating the grid in Main allows you to pass it anywhere (without accessing the view).

---

The MouseListener is rather long, mainly because it does things it shouldn't do. It should compute the coordinates and call something like gameState.playAt(row, col).

protected void paintComponent(Graphics g) ...

This is a pretty long method composed of 4 main parts separated by comments. I'd probably extract the parts and drop the comments.

##Grid

I'd rename it to GameState and add currentPlayer and lastMove (or a whole history).

private final int SIZE;

This is no constant and should be called size.

private Stone[][] stones;

I'm not sure if storing so much information is needed/useful. Simply storing State should do. I usually try to create a minimal model and store redundant information in other variables as needed.

You're missing a check if the move is legal, i.e., 1. your new group has at least one liberty after possible captures have been evaluated, 2. it's not a ko.

public void checkStone(Stone stone) ...

I don't like the name. What about removeGroupIfDead?

##Chain

 public ArrayList<Stone> stones;
 public State state;

Avoid public variables. First try moving everything what needs to access the class members into the class itself, otherwise provide some accessors (but don't add a getter for collections as it allows the caller to modify them later).

public int getLiberties() {
 int total = 0;
 for (Stone stone : stones) {
 total += stone.liberties;
 }
 return total;
}

You're counting some liberties multiple times, which is ugly, but alright for what you're using it, namely testing if there's any. So a boolean result should do.

It looks like using Chain doesn't buy you much as you have to iterate anyway. Being able to store the liberties could make the evaluation fast (needed for a AI player), but this may be hard to do. Iterating over all neighbors every time could possibly be done without any Chain/Stone.

##Stone

Again, public variables. I'd try to get rid of the whole class, but this may be hard.