Basic bingo game in Java

Question

I've recently wrote a simple bingo game in Java to refresh myself in oop principals I have not touched in quite a while. I feel like I have accomplished this, but I would like to learn as much as possible from this exercise. Besides the oop principals, I tried to make the code very readable and reusable in case there was ever a 7x7 or a 3x3 version of bingo, and I also tried to eliminate magic numbers. Is there anything that I should do differently or improve on?

BingoBoard.java

package bingoboard;
/**
 *
 * @author Dom
 */
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Random;
public class BingoBoard 
{
 private String board[][];
 private final int BOARD_DIM = 5;
 private final int MAX_SIZE = BOARD_DIM * BOARD_DIM;
 private HashMap<String,Boolean> eventCalledMap;
 private ArrayList<String> events;
 private ArrayList<String> selectedEvents;
 private final String FREE = "FREE SPACE";
 private final int player;
 private boolean win;
 BingoBoard()
 {
 board = new String[BOARD_DIM][BOARD_DIM];
 selectedEvents = new ArrayList<>();
 events = new ArrayList<>();
 eventCalledMap = new HashMap<>();
 eventCalledMap.put(FREE, true);
 player = -1;
 win = false;
 }//end BingoBoard
 BingoBoard(ArrayList<String> eventList)
 {
 board = new String[BOARD_DIM][BOARD_DIM];
 selectedEvents = new ArrayList<>();
 events = eventList;
 eventCalledMap = new HashMap<>();
 eventCalledMap.put(FREE, true);
 player = -1;
 win = false;
 }//end BingoBoard
 BingoBoard(ArrayList<String> eventList, int numb)
 {
 board = new String[BOARD_DIM][BOARD_DIM];
 selectedEvents = new ArrayList<>();
 events = eventList;
 eventCalledMap = new HashMap<>();
 eventCalledMap.put(FREE, true);
 player = numb;
 win = false;
 }//end BingoBoard
 //updates the event list.
 public void updateEvents(ArrayList<String> eventList)
 {
 events.addAll(eventList);
 }//end updateEvents
 //Chooses events and adds them to the board.
 public boolean randomizeEvents()
 {
 if(this.events.size() < MAX_SIZE - 1)
 return false;
 while(selectedEvents.size() < MAX_SIZE - 1)
 {
 Random rand = new Random();
 int index = rand.nextInt(this.events.size());
 String str = events.get(index);
 selectedEvents.add(str);
 events.remove(str);
 }//end while
 int count = 0;
 for(String str:selectedEvents)
 {
 eventCalledMap.put(str,false);
 if(count == MAX_SIZE/2)
 { 
 board[count/BOARD_DIM][count%BOARD_DIM] = FREE;
 count++;
 }//end if
 board[count/BOARD_DIM][count%BOARD_DIM] = str;
 count++;
 }//end for
 return true;
 }//end randomizeEvents
 public void printBoard()
 {
 System.out.printf("Player %d\n",this.player);
 System.out.println("_____________________");
 for(int i = 0; i < BOARD_DIM; i++)
 {
 System.out.println("|---|---|---|---|---|");
 for(int j = 0; j < BOARD_DIM; j++)
 if(eventCalledMap.get(board[i][j]) == true)
 System.out.printf("|%3s", "X");
 else
 System.out.printf("|%3s",board[i][j]);
 System.out.println("|");
 }//end for
 System.out.println("|---|---|---|---|---|");
 System.out.println("_____________________\n\n");
 }//end printBoard
 //Puts maker on given value if it
 public void putMarker(String value)
 {
 if(eventCalledMap.containsKey(value))
 eventCalledMap.put(value, Boolean.TRUE);
 }//end method putMarker
 /*Checks board for a win and returns true if board won and false 
 otherwise. */
 public boolean checkWin()
 {
 this.win = evalBoard();
 return this.win;
 }//end method putMarker
 //Returns true if
 public boolean won()
 {
 return this.win;
 }//end method won
 //returns player number
 public int getPlayer()
 {
 return player;
 }//end getPlayer
 //Checks the board for win. Returns true if a win is found.
 private boolean evalBoard()
 {
 int i, j, count;
 for(i = 0; i < BOARD_DIM; i++)
 {
 j = 0;
 count = 0;
 //Checks horizontally for a win.
 while(eventCalledMap.get(board[i][j]) != false)
 {
 count++;
 j++;
 if(count == BOARD_DIM)
 return true;
 }//end while
 j = 0;
 count = 0;
 //Checks verically for a win. 
 while(eventCalledMap.get(board[j][i]) != false)
 {
 count++;
 j++;
 if(count == BOARD_DIM)
 return true;
 }//end while
 }//end for
 i = 0;
 count = 0;
 //Checks the top left to bottom right diagnal for a win.
 while(eventCalledMap.get(board[i][i]) != false)
 {
 count++;
 i++;
 if(count == BOARD_DIM)
 return true;
 }//end while
 i = BOARD_DIM -1;
 j = 0;
 count = 0;
 //Checks the top left to bottom right diagnal for a win. 
 while(eventCalledMap.get(board[i][j]) != false)
 {
 count++;
 i--;
 j++;
 if(count == BOARD_DIM)
 return true;
 }//end while
 return false;
 }//end evalBoard
}//end class

BingoGame.java

package bingoboard;
import java.util.ArrayList;
import java.util.Scanner;
/**
 *
 * @author Dom
 */
public class BingoGame
{
 private ArrayList<String> eventList;
 private final int DEFAULT_PLAYER_COUNT = 2;
 private int playerCount;
 private boolean winnerDetermined;
 private ArrayList<BingoBoard> boardList;
 BingoGame()
 {
 this.eventList = new ArrayList<>();
 this.playerCount = DEFAULT_PLAYER_COUNT;
 this.winnerDetermined = false;
 this.boardList = new ArrayList<>();
 }//end default constructor 
 BingoGame(int players)
 {
 this.eventList = new ArrayList<>();
 this.playerCount = players;
 this.winnerDetermined = false;
 boardList = new ArrayList<>();
 }//end constructor 
 //adds events for game.
 public void addEvent(String event)
 {
 this.eventList.add(event);
 }//end method addEvent
 //Main driver for the game.
 public void startGame()
 {
 this.winnerDetermined = false;
 for(int i = 1; i <= this.playerCount;i++)
 {
 ArrayList<String> events = (ArrayList<String>) eventList.clone();
 BingoBoard board = new BingoBoard(events,i);
 board.randomizeEvents();
 this.boardList.add(board);
 board.printBoard();
 }//end for
 Scanner in = new Scanner(System.in);
 while(this.winnerDetermined == false)
 {
 System.out.println("Enter Event:");
 String check = in.next();
 for(BingoBoard boards:boardList)
 {
 boards.putMarker(check);
 boards.printBoard();
 if(winnerDetermined == false)
 winnerDetermined = boards.checkWin();
 else
 boards.checkWin();
 }//end for
 }//end while
 this.printWinner();
 }//end startGame
 //Prints out winning boards. More than one player may win.
 private void printWinner()
 {
 //Prints out winning boards. More than one player may win.
 for(BingoBoard boards:boardList)
 {
 if(boards.won())
 System.out.printf("Player %d wins!\n\n",boards.getPlayer());
 }//end for 
 }//end printWinner
}//end class

BingoTester.java

package bingoboard;
/**
 *
 * @author Dom
 */
public class BingoTester {
 /**
 * @param args the command line arguments
 */
 public static void main(String[] args) 
 {
 BingoGame game = new BingoGame(4);
 for(int i=1; i<=25; i++)
 game.addEvent(Integer.toString(i));
 game.startGame();
 }//end main
}//end class

Answer 1

• Initialize variables inline where you can, to reduce boilerplate: private String board[][] = new String[BOARD_DIM][BOARD_DIM];, etc.

• Don't use default scoping unless you really mean to. Prefer public or private, as appropriate.

• Delegate from one constructor to another, where you can.

  public BingoBoard(ArrayList<String> eventList)
  {
   this();
   updateEvents(eventList);
  }
  

• If you are going to add per-method comments, might as well teach yourself javadoc while you're at it:

  /**
   * Chooses events and adds them to the board.
   */
  public boolean randomizeEvents() {
  

• Putting Random rand = new Random(); inside the loop is wasteful of resources, and will occasionally cause nextInt() to return the same value on consecutive occasions due to reseting the random number generator, rather than getting the next number from the same generator. Move it up, outside the loop.

• This block deserves a comment, or better, to be moved to a self-documenting method. It looks like a bug to me (if BOARD_DIM is 5, then this is executed on the 12th event.. board[2][3] = FREE;.. really? Is that what you want to do?).

  if(count == MAX_SIZE/2)
  { 
   board[count/BOARD_DIM][count%BOARD_DIM] = FREE;
   count++;
  }//end if
  

• Review all your comments. Some are out of date.

• Why is there a won() and checkWin() method? And if won() is really, really needed, why doesn't checkWin() call it?

• evalBoard() shouldn't be necessary. When creating the board, determine how many squares must be marked in order to win; when putMarker() puts a marker (inside the containsKey conditional), increment a markers counter; when markers == markersRequired then the board is won. Also, it's swarming with nested conditionals and loops. That is major code smell.

• Rename startGame(). That method plays the entire game, it doesn't just start it. In fact, it's probably best to split that method up a bit, separate its concerns a bit. prepareBoard(), pullNumber(), checkNumberAndPlaceMarker(), things like that might work.

• Dom: 3 things: 1. evalBoard() is necessary because there are no set number of markers you need to win the game it can be anywhere from 5 to 17 markers. If there is a way to make it better I am all ears, but it is necessary. 2. You're right about needing to document some things and you got that I wanted to put the free space in the middle, but I can tell you didn't run the code because it always puts the free space at board[2][2] not board[2][3] and it is designed that way for a reason. If you cut any odd number in half that will be the middle index counting from zero where you want the free space.
• Paul Hicks: @Dom 1. You mean two people could be playing the same game of bingo and one might need three times as many markers as another? Hardly seems fair! You should require the number of markers as either a constant, or as a parameter to the game. 2. Yes I didn't run the code, I reviewed it. Ok, so the free space is put in the right place, that's good. It's also completely cryptic! Hence the need for a comment or separate method.
• abuzittin gillifirca: Event based programs; games, graphics, gui etc; have some kind of loop in the start up method, (although it may be hidden by runtime). It is Expected that program read something like {setUp(); mainLoop(); /*optionally*/tearDown();} Some win32 programs may call the loop routine PumpMessages() some OpenGl program may call it startEventLoop(). @Dom need not really name playEntireGameAndDontFinishUntilItsOver as a simple rename from startGame() to startGameLoop or similar would be enough.
• abuzittin gillifirca: @PaulHicks calling Random rand = new Random(); rand.nextInt() in a loop to get a stream of random integers is just wrong. You should use a more forceful language there, it is not a matter of taste.

Answer 2

1. The BingoBoard class does not fulfill the single responsibility principle. It handles the table and also prints it. I'd extract out the printing logic (and other IO logic too) to a separate class. This would make unit-testing easier and help if you want to use a graphical or web UI instead of console. (After that you might notice other responsibilities too which could be moved to separate classes.)

2. It would be cleaner if you don't use System.out directly. Writing to a generic PrintStream or a Writer could enable writing to a network socket etc., and also would make testing easier.

3. private ArrayList<String> eventList;
   

   ArrayList<...> reference types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces

   The same is true for HashMap (Map).

4. Field declaration and assigning to it an initial value could be on the same line in some cases, like:

   private final List<String> eventList = new ArrayList<>();
   

   It would remove some duplication from constructors.

5. Instead of cloning and casting you could use a copy constructor:

   final List<String> events = new ArrayList<String>(eventList);
   

6. final List<String> events = new ArrayList<String>(eventList);
   final BingoBoard board = new BingoBoard(events, i);
   

   I'd move input list copying to the BingoBoard class. (Effective Java, 2nd Edition, Item 39: Make defensive copies when needed)

7. It would be cleaner to call close on the Scanner instance.

8. The winnerDetermined flag could be declared inside the startGame method. The startGame method override its value at the beginning and no other method reads it. (Effective Java, Second Edition, Item 45: Minimize the scope of local variables)

9. Furthermore, this flag could completely omitted with a break statement:

   outer: while (true) {
    System.out.println("Enter Event:");
    final String check = in.next();
    for (final BingoBoard boards: boardList) {
    boards.putMarker(check);
    boards.printBoard();
    final boolean checkWin = boards.checkWin();
    if (checkWin) {
    break outer;
    }
    }
   }
   

   (I've not tested this refactoring.) To be honest, the need of a label smells here but I think a few more refactoring steps would help. (For example, extracting out the inner loop to a separate method.)

10. You could use printWinner() instead of this.printWinner().

11. player = -1;
    

    -1 is magic number. Using named constants instead of numbers would make the code more readable and less fragile.

12. Comments on the closing curly braces are unnecessary and disturbing. Modern IDEs could show blocks.

     }// end evalBoard
    }// end class
    

    "// ..." comments at end of code block after } - good or bad?

Answer 3

In addition to the previous answer, I'd like to add:

The evalBoard() method is too long and contains duplicates. I'd extract a method:

/**
 * Checks the board for win
 * @return true if a win is found
 */
private boolean evalBoard() {
 for (int i = 0; i < BOARD_DIM; i++) {
 // Checks horizontally for a win.
 if (evalLine(0, i, 1, 0)) {
 return true;
 }
 // Checks vertically for a win.
 if (evalLine(i, 0, 0, 1)) {
 return true;
 }
 }
 // Checks the top left to bottom right diagonal for a win.
 if (evalLine(0, 0, 1, 1)) {
 return true;
 }
 // Checks the top right to bottom left diagonal for a win.
 if (evalLine(BOARD_DIM-1, 0, -1, 1)) {
 return true;
 }
 return false;
}
private boolean evalLine(int startx, int starty, int deltax, int deltay) {
 int count = 0;
 int x = startx;
 int y = starty;
 while (!eventCalledMap.get(board[x][y])) {
 x += deltax;
 y += deltay;
 if (++count == BOARD_DIM) {
 return true;
 }
 }
 return false;
}