patternjavaModerate
Basic bingo game in Java
Viewed 0 times
javagamebingobasic
Problem
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 eventCalledMap;
private ArrayList events;
private ArrayList 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 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 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
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 eventCalledMap;
private ArrayList events;
private ArrayList 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 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 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
Solution
-
Initialize variables inline where you can, to reduce boilerplate:
-
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.
-
If you are going to add per-method comments, might as well teach yourself javadoc while you're at it:
-
Putting
-
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..
-
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
-
Rename
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 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.Code Snippets
public BingoBoard(ArrayList<String> eventList)
{
this();
updateEvents(eventList);
}/**
* Chooses events and adds them to the board.
*/
public boolean randomizeEvents() {if(count == MAX_SIZE/2)
{
board[count/BOARD_DIM][count%BOARD_DIM] = FREE;
count++;
}//end ifContext
StackExchange Code Review Q#40497, answer score: 11
Revisions (0)
No revisions yet.