HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Reversi game state in Android

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamereversistateandroid

Problem

I redid a Reversi game state class that is online. It follows the conventional rules of the board game. The game state connects to a command line interface now. I am unfamiliar with Android so comments related to that would be especially appreciated. Java related comments would be useful too.

```
/**
* Date: 11/3/13
* Time: 11:07 AM
* based on https://github.com/dweekly/android-reversi
*/
public class GameState {
public static final byte DIM = 8;
public static final byte MAXRAYS = 8;
public static final char LIGHT = 'W';
public static final char DARK = 'B';
private final static char[][] DEFAULTBOARD = {
{' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
{' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
{' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
{' ', ' ', ' ', 'W', 'B', ' ', ' ', ' '},
{' ', ' ', ' ', 'B', 'W', ' ', ' ', ' '},
{' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
{' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '},
{' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '}
};
private char _board[][];
private char _currentSide;
private boolean _frozen;

public GameState() {
this(DEFAULTBOARD, DARK);
}

public GameState(char board[][], char currentSide) {
_board = new char[DIM][DIM];
transferBoard(_board, board);
_currentSide = currentSide;
_frozen = false;
}

public void freeze(boolean state) {
_frozen = state;
}

public void transfer(GameState copy) {
assert(!_frozen);
_currentSide = copy._currentSide;
transferBoard(_board, copy._board);
}

public void swapSides() {
assert(!_frozen);
assert (Location.isaSide(_currentSide));
_currentSide = (_currentSide == LIGHT) ? DARK : LIGHT;
}

private static void transferBoard(char[][] board1, char[][] board2) {
for (byte x = 0; x 1) {
for (int backStep = steps

Solution

Your code looks very much like something one would write in C. On the one hand this is good, because you choose fairly efficient solutions. On the other hand, you throw away many powerful features of Java that would make your code simpler and safer. I'll just present a couple of spotlights that should get you going.

Java is Garbage Collected

This means you don't have to keep track of object ownership. For example, you can create a new array inside a method and return it without any problems. This is better than passing empty arrays as function arguments which you then populate. E.g. in makeTurners, the turners argument is unnecessary.

Arrays != Pointers

While arrays are a language primitive of Java, they are more sophisticated than their C counterpart. For example, they carry their length around with them: someArray.length.

The Java Collection Framework

Arrays suck, and are not useful in most use cases (this is true in C++ as well, where the Standard Library provides superior alternatives to C arrays). The main problem with arrays is that they are fixed-sized, which forces you to carry the highest index around in variable-length situations. In Java, you can find lots of useful Collection classes in the java.util namespace. In most cases where you'd think “array”, use an ArrayList instead. They support parametric polymorphism aka. generics. You can instantiate a new instance like:

List myList = new ArrayList();


where Integer is the type of object inside that collection. Note that primitive types like char or int cannot be used here, because they aren't objects. However, there are wrapper classes like Character and Integer. In many cases, primitives are promoted to the wrapper type, so you don't have to care.

You can add a new element to a collection using the add method:

myList.add(42); // 42 gets promoted to an Integer


The List interface describes certain functionality that all Lists implement, e.g. ArrayLists and LinkedLists. For example, this mandates a get method, so that we can access out elements:

Foo fooFromTheList = myList.get(0);


You can get the size of a collection like myList.size().

There are many more collections like various Sets and Maps.

Iteration

The built-in collections and the dumb arrays can be iterated over using a for-each type loop. This is useful whenever you don't really care about the indices. For example:

// with plain arrays
 int[] someInts = { 1, 42, 12 };
 for (int i : someInts) {
   System.out.println(i);
 }

 // with an array list
 List moreInts = new ArrayList();
 moreInts.add(1);
 moreInts.add(42);
 moreInts.add(12);

 for (int i : moreInts) {
   System.out.println(i);
 }


Java is Object Oriented

Currently, you are only using classes to namespace your functions. This is a good start. However, they are also useful as supercharged C structs, that is: to group data together.

For example, you could group the x and y to a single Location:

class Location {
  public in x, y; // here we declare member fields

  // this is a *constructor*
  public Location(int x, int y) {
    this.x = x;
    this.y = y;
  }
}


We can now create a new Location like Location overThere = new Location(x, y). We can access the fields like overThere.x. Other functions that are only about locations all belong in that class.

Another class you should create is a Board, e.g.:

class Board {
  public char[][] board;
  public int      size;

  public Board(char[][] board) {
    this.size = board.length;
    // ... copy the primitive board over here
  }

  // ... utility methods
}


This class would then contain methods that are only about the Board, not about a Location or a GameState. Examples are field access and bounds checking, which belongs here. The Board should also have a toString() method which gets called when you try to print out a Board. The default board also belongs here, not into the game state.

The GameState would then have a member field containing a Board instance.

Single Responsibility Principle (SRP) and other design guidelines

The SRP is an object-oriented design principle that tells us that each class should have one single task. Any further functionality does not belong there. When I look at your code, I see various things that should be classes of their own:

  • Already mentioned: Board, Location.



  • Use a class for your Sides instead of chars.



  • Use a class for your Fields instead of chars.



-
Maybe use a class to represent each Move, rather than implicitly encoding this in the GameState. A Move would then have a source GameState, and lead to another one. It has a Location where a player places a disk, and a List of all discs that will be flipped by this move. What is the advantage of this abstraction?

  • Because it records the prior state, you can easily implement an undo operation.



  • This representation has the effect that ea

Code Snippets

List<Integer> myList = new ArrayList<Integer>();
myList.add(42); // 42 gets promoted to an Integer
Foo fooFromTheList = myList.get(0);
// with plain arrays
 int[] someInts = { 1, 42, 12 };
 for (int i : someInts) {
   System.out.println(i);
 }

 // with an array list
 List<Integer> moreInts = new ArrayList<Integer>();
 moreInts.add(1);
 moreInts.add(42);
 moreInts.add(12);

 for (int i : moreInts) {
   System.out.println(i);
 }
class Location {
  public in x, y; // here we declare member fields

  // this is a *constructor*
  public Location(int x, int y) {
    this.x = x;
    this.y = y;
  }
}

Context

StackExchange Code Review Q#33780, answer score: 4

Revisions (0)

No revisions yet.