Return to Answer

• 1

• 30.3k
• 9
• 82
• 157

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).

1. 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.

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

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

2.  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)

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

2. 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)

3. 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.)

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

2.  player = -1;
   

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

1. 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?