[CS2113-T18-1] ChessMaster #19

ken-ruster · 2023-10-05T09:34:33Z

ChessMasterCLI is a command-line interface (CLI) chess game designed to make learning and training accessible for beginners while offering an engaging experience for all skill levels. This sleek and user-friendly chess simulator provides a platform for novice players to build their skills and understanding of the game.

Test Storage

maanyos

Good organisation of packages, use of exceptions, and documentations!

maanyos · 2023-10-22T06:24:31Z

src/main/java/chessmaster/user/Player.java

+
+import java.util.ArrayList;
+
+public abstract class Player {


good choice of abstract class for player!

maanyos · 2023-10-22T06:26:58Z

src/main/java/chessmaster/user/Player.java

+            row = 0;
+        }
+
+        for (int tempRow = row; row < tempRow + 2; row++) {


would make more sense if tempRow was changed instead of row

maanyos · 2023-10-22T06:29:19Z

src/main/java/chessmaster/user/Player.java

+    public Move getNextMove(ChessBoard board) throws InvalidMoveException, NullPieceException, ParseCoordinateException{
+        String input = TextUI.getUserInput(); // Get user input
+        try {
+            Parser parser = new Parser();
+            Command command = parser.parseCommand(input, board);
+            if (command instanceof AbortCommand) {
+                return null;
+            }
+            return command.getMove();
+
+        } catch (ParseCoordinateException | NullPieceException e) {
+            TextUI.printErrorMessage(e);
+        }
+        return new Move();
+    }


is the catch clause intended to throw the same exception again after printing error message?

maanyos · 2023-10-22T06:33:10Z

src/main/java/chessmaster/parser/Parser.java

+        int colour = promoteFrom.getColour();
+        Coordinate position = promoteFrom.getPosition();
+
+        switch (promoteTo.toLowerCase()){


good versatility

maanyos · 2023-10-22T06:41:38Z

src/main/java/chessmaster/pieces/ChessPiece.java

+
+import java.util.ArrayList;
+
+public abstract class ChessPiece {


good use of abstract class for chess piece!

maanyos · 2023-10-22T06:48:17Z

src/main/java/chessmaster/storage/Storage.java

+    public static void saveBoard(ChessBoard board) throws SaveBoardException {
+
+        try (FileWriter fileWriter = new FileWriter(filePath)){
+            for (int row = 0; row < 8; row++) {


try do define constants as final variables on top

maanyos

Good that you've added help messages
Good use of documentation and comments in some parts to explain code
Try to keep the way of printing messages consistent, some use const final strings, others put the string directly in the methods
Keep the way of printing output consistent also, some classes are directly calling system.out.print

maanyos · 2023-10-28T06:33:40Z

src/main/java/chessmaster/ChessMaster.java

+    public static void main(String[] args) {
+        new ChessMaster().run();
+    }


good abstraction of main

maanyos · 2023-10-28T06:38:22Z

src/main/java/chessmaster/commands/MoveCommand.java

+        if (move == null) {
+            throw new InvalidMoveException();
+        }


might be more informative if you add a message argument to the exception or use a different exception
can also consider using assertion if this is intended to ensure code correctness

maanyos · 2023-10-28T06:50:07Z

src/main/java/chessmaster/game/ChessBoard.java

+import chessmaster.pieces.Pawn;
+import chessmaster.ui.TextUI;
+
+public class ChessBoard {


this class is doing a lot!
consider extracting parts that deal with pieces and moves

maanyos · 2023-10-28T06:59:04Z

src/main/java/chessmaster/game/MiniMax.java

+        //gets all the moves for the current player
+        ChessBoard board = tuple.getBoard();
+        Move[] moves = board.getAllMoves(color);
+        assert moves.length > 0 : "No moves available for " + color + " at depth " + depth;


can moves.length = 0 ever happen during use?

maanyos · 2023-10-28T07:00:17Z

src/main/java/chessmaster/game/Move.java

+        assert from != null && to != null : "Coordinates in Move should not be null!";
+        assert piece != null && !piece.isEmptyPiece() : "Chess piece in Move should not be null or empty!";


can these happen during run time?

maanyos · 2023-10-28T07:01:41Z

src/main/java/chessmaster/pieces/ChessPiece.java

+    private int[][] boardWeight = 
+        {{0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0},
+        {0,0,0,0,0,0,0,0}};


no need to initialise here right

maanyos · 2023-10-28T07:07:29Z

src/main/java/chessmaster/pieces/ChessPiece.java

+        ArrayList<Coordinate> flattenedCoordinates = new ArrayList<>();
+
+        for (Coordinate[] direction : availableCoordinates) {
+            for (Coordinate possibleCoord : direction) {
+                if (this.isMoveValid(possibleCoord, board)) {
+                    flattenedCoordinates.add(possibleCoord);
+                }
+            }


consider refactoring this part if its used in many methods

azfarulmatin · 2023-11-02T09:09:03Z

docs/DeveloperGuide.md

@@ -23,16 +137,29 @@
 |Version| As a ... | I want to ... | So that I can ...|


is it supposed to be formatted this way?

azfarulmatin · 2023-11-02T09:15:27Z

docs/DeveloperGuide.md

+**API:**
+
+Below is a class diagram representing the Storage class.
+The Storage component is responsible for handling the storage and retrieval of chess game state.


Should the StorageClass diagram be shown at the next line instead?

azfarulmatin · 2023-11-02T09:28:44Z

docs/DeveloperGuide.md

+
+![](StorageSequence.png)
+
+* Creates the necessary parent directories for the file and the file itself if they don't exist 


It would be helpful to provide more detailed explanations alongside the diagrams to offer readers a better understanding of creating parent directories, saving the ChessBoard state, resetting the game, and loading the state.

azfarulmatin · 2023-11-02T09:31:04Z

docs/DeveloperGuide.md

+In order to handle user input into the program during the game, the `Parser` class was implemented.
+Below is a sequence diagram describing the process of handling user input passed from `Game`:
+
+![](images/ParseCommandSequence.png)


Great job on the diagram and explanation of how the Parser class works! The visual representation along with the detailed step-by-step breakdown makes it easy to understand the process.

DextheChik3n

Added review comments for DG

DextheChik3n · 2023-11-02T09:36:02Z

docs/StorageSequence.png

This sequence diagram seems too big, perhaps break down into the different method calls

DextheChik3n · 2023-11-02T09:41:45Z

docs/images/ParseCommandSequence.png

is there a way to prevent the red cross from overlapping with the String return arrow? I feel that this could be misinterpreted

DextheChik3n · 2023-11-02T09:43:57Z

docs/StorageSequence.png

is there supposed to be a method call here?

DextheChik3n · 2023-11-02T09:49:45Z

docs/StorageSequence.png

Perhaps colon was forgotten before the class name 😅

ziyi105

Overall clean and neat!

ziyi105 · 2023-11-02T09:17:28Z

docs/diagrams/ChessMasterSequence.puml

+
+    chessMaster -> ui : shouldStartNewGame()
+    activate ui
+    user -> ui : "yes/no"


Is this supposed to be the condition? Because its meaning kinda overlaps with Previous Board Exist, so maybe you should just use one of them

ziyi105 · 2023-11-02T09:21:57Z

docs/diagrams/ParseCommandSequence.puml

+    Command -> ChessBoard ++: showAvailableCoordinates
+    ChessBoard --
+    Command -> ChessBoard ++: getAvailableCoordinatesString
+    ChessBoard --> Command --: String


Perhaps using coordinateString: String is clearer?

ziyi105 · 2023-11-02T09:35:00Z

docs/StorageSequence.puml

+
+
+create Storage
+User -> Storage++ : new Storage(): String


I am not sure how your code is implemented, but it seems a bit strange to me to have user calling the command to create a new Storage() object. Did you mean user will input a command which is received by a class and the class triggers the creation of storage object? If yes, what is the class that triggers the creation of storage object? Perhaps in your uml you should have an arrow pointing from the class to storage class instead?

ziyi105 · 2023-11-02T09:37:03Z

docs/StorageSequence.puml

+Storage -> Storage++ : createChessMasterFile()
+Storage --> Storage--
+create FileWriter
+Storage -> FileWriter++ : new FileWriter(): File


Is there supposed to be a lifeline for FileWriter?

Start bug fix

Add `isChecked` and `isCheckMated` features

Fix minimax aggro

Fix User Guide formatting

Fix wrong color when player loses

Fix empty payload in moves command

Update

Update docs

Update PPP

Ignore empty spaces in each `nextLine` method in `Storage` class

Update onx001.md

Fix bug with current turn in save

…into tricia

Add PPP link in AboutUs

Remove chessboard in storage

Remove json tag in code block

…-tests

Add tests

Add gameplay link

ken-ruster changed the title ~~[T18-1] ChessMaster~~ [CS2113-T18-1] ChessMaster Oct 5, 2023

Brian030601 pushed a commit to Brian030601/tp that referenced this pull request Oct 19, 2023

Merge pull request nus-cs2113-AY2324S1#19 from junhyeong0411/master

b6dd7c8

Test Storage

maanyos reviewed Oct 22, 2023

View reviewed changes

maanyos reviewed Oct 28, 2023

View reviewed changes

azfarulmatin reviewed Nov 2, 2023

View reviewed changes

DextheChik3n reviewed Nov 2, 2023

View reviewed changes

ziyi105 reviewed Nov 2, 2023

View reviewed changes

TongZhengHong and others added 23 commits November 3, 2023 17:43

Merge pull request #115 from onx001/StartBug

9fc8c13

Start bug fix

Fix minimax aggro

70b7003

Update MiniMax.java

e0159f0

Merge pull request #116 from onx001/Check

f2e8235

Add `isChecked` and `isCheckMated` features

Merge pull request #149 from onx001/MinimaxAggro

10847a1

Fix minimax aggro

Update no available moves ui

37cbf93

Fix empty payload in ShowMovesCommand

3268804

Fix wrong color when player loses

98331c4

Convert code blocks from markdown to html

c0b3577

Fix link to legend command

0fe8b95

Merge pull request #152 from TongZhengHong/fix-ug-formatting

80e109a

Fix User Guide formatting

Merge pull request #151 from TongZhengHong/fix-wrong-losing-color

f17486a

Fix wrong color when player loses

Merge pull request #150 from TongZhengHong/fix-moves-command

dc3668c

Fix empty payload in moves command

Add InvalidCommand class

f7838f2

Update NullPieceException message

7eb0f9e

Check for empty move

f8a7b35

Merge pull request #15 from AY2324S1-CS2113-T18-1/master

714d012

Update

Merge branch 'master' of https://github.com/AY2324S1-CS2113-T18-1/tp

d82392e

Add function to save move history

1f3e7f7

Update documentation

498fbea

Add expected output to UserGuide

08420ba

Check hasNextLine in getUserInput

7324ef4

Add CPU thinking message

a052e85

TongZhengHong and others added 30 commits November 14, 2023 11:13

Fix UI message for choose player color

640f78c

Check empty spaces and delimiter in Storage class

6afb108

Merge pull request #195 from onx001/docs3

67be4cc

Update docs

Merge pull request #194 from ken-ruster/add-tests

aae3ad2

Update PPP

Merge pull request #196 from TongZhengHong/branch-rebust-storage

04e18cb

Ignore empty spaces in each `nextLine` method in `Storage` class

Update onx001.md

50c96b0

Fix bug with current turn in save

a7aa3a1

Add PPP link

9fa6733

Merge branch 'master' into add-tests

59f5d20

Merge pull request #197 from onx001/ppp1

ad9de0c

Update onx001.md

Merge pull request #198 from ken-ruster/add-tests

aa788a2

Fix bug with current turn in save

Merge branch 'master' of https://github.com/AY2324S1-CS2113-T18-1/tp …

d7bb443

…into tricia

Update PPP links in AboutUs

8ffe6a4

Minor fixes

5cae662

Update PPP

460dc3b

Disable loadBoard method and remove saving

3d190c1

Add link to TP Dashboard

6f75922

Remove calls to loadBoard

c9dc296

Fix checkstyle

65fccd0

Fix checkstyle 2

e630390

Merge pull request #199 from TriciaBK/tricia

0fe9f19

Add PPP link in AboutUs

Fix checkstyle 3

f895863

Merge pull request #200 from TongZhengHong/remove-chessboard-storag

21b3717

Remove chessboard in storage

Remove json tag in code block

d74d6d3

Fix bug with current turn in save

bf7e53a

Merge pull request #201 from TongZhengHong/branch-ug-format-4

7a30a43

Remove json tag in code block

Merge branch 'add-tests' of https://github.com/ken-ruster/tp into add…

92a817f

…-tests

Merge pull request #202 from ken-ruster/add-tests

640bcaf

Add tests

Add gameplay link

1259479

Merge pull request #203 from TongZhengHong/branch-ug-format-4

a4245f1

Add gameplay link

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CS2113-T18-1] ChessMaster #19

[CS2113-T18-1] ChessMaster #19

ken-ruster commented Oct 5, 2023

maanyos left a comment

maanyos Oct 22, 2023

maanyos Oct 22, 2023

maanyos Oct 22, 2023

maanyos Oct 22, 2023

maanyos Oct 22, 2023

maanyos Oct 22, 2023

maanyos left a comment

maanyos Oct 28, 2023

maanyos Oct 28, 2023

maanyos Oct 28, 2023

maanyos Oct 28, 2023

maanyos Oct 28, 2023

maanyos Oct 28, 2023

maanyos Oct 28, 2023

azfarulmatin Nov 2, 2023

azfarulmatin Nov 2, 2023

azfarulmatin Nov 2, 2023

azfarulmatin Nov 2, 2023

DextheChik3n left a comment

DextheChik3n Nov 2, 2023

DextheChik3n Nov 2, 2023

DextheChik3n Nov 2, 2023

DextheChik3n Nov 2, 2023

ziyi105 left a comment

ziyi105 Nov 2, 2023

ziyi105 Nov 2, 2023

ziyi105 Nov 2, 2023

ziyi105 Nov 2, 2023


		import java.util.ArrayList;

		public abstract class ChessPiece {

		assert from != null && to != null : "Coordinates in Move should not be null!";
		assert piece != null && !piece.isEmptyPiece() : "Chess piece in Move should not be null or empty!";

		@@ -23,16 +137,29 @@
		\|Version\| As a ... \| I want to ... \| So that I can ...\|


		![](StorageSequence.png)

		* Creates the necessary parent directories for the file and the file itself if they don't exist

[CS2113-T18-1] ChessMaster #19

Are you sure you want to change the base?

[CS2113-T18-1] ChessMaster #19

Conversation

ken-ruster commented Oct 5, 2023

maanyos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maanyos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DextheChik3n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ziyi105 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment