Feat/level1 #1
Feat/level1 #1
Conversation
src/main/java/view/InputView.java
Outdated
| public static String readFirstCommand() { | ||
| try { | ||
| String command = scanner.nextLine(); | ||
| inputValidation.validateFirstCommand(command); |
src/main/java/domain/Command.java
Outdated
| public static Command from(final String command) { | ||
| return Arrays.stream(Command.values()) | ||
| .filter(status -> command.startsWith(status.select)) | ||
| .findAny().orElseThrow(() -> new IllegalStateException(WRONG_INPUT.getMessage())); |
|
|
||
| @DisplayName("해당 기물의 이름을 반환한다.") | ||
| @Test | ||
| void getName() { |
There was a problem hiding this comment.
많은 테스트는 좋지만 getter/setter도 테스트를 할 이유가 있을까?
There was a problem hiding this comment.
많은 테스트는 좋지만 getter/setter도 테스트를 할 이유가 있을까?
저도 이부분에 대해 많이 생각했습니다. 현재 Getter의 경우 단순하게 객체의 속성 값을 반환하는 로직이기에 굳이 필수 테스트는 아니라고 생각합니다. 다만 현재 반환되는 값이 매우 중요한 정보를 담고있기에(이름에 따라 움직임이 달라짐) 테스트를 넣게 되었습니다.
src/test/java/domain/BoardTest.java
Outdated
|
|
||
| //when & then | ||
| assertThatThrownBy(() -> board.movePieces(preLocation, moveLocation)) | ||
| .isInstanceOf(IllegalStateException.class); |
There was a problem hiding this comment.
현재 구조에서 예외메시지 검증은 필요 없을까??
모두 같은 예외를 던지기때문에 예외메시지 테스트도 중요한것같습니다.
| @DisplayName("비숍이 이동할수 있는지 확인한다.") | ||
| @CsvSource({"d, 4, true", "d, 2, true", "c, 4, false", "a, 1, true", "c, 5, false"}) | ||
| void canMove(final char moveRank, final char moveFile, final boolean canMove) { | ||
| //given |
There was a problem hiding this comment.
이건 개인적인 의견인데
//given 보다는 // given 이것처럼 한 칸 띄워주는게 더 보기는 좋을듯!
ex. abc, def, gh...
| String preLocation = inputInfo[PRE_INDEX]; | ||
| String moveLocation = inputInfo[MOVE_INDEX]; | ||
|
|
There was a problem hiding this comment.
이 부분도 객체로 관리하면 어떤 점이 좋을까?
복잡한 로직을 간단하게 표현할수 있고, 추가로 예외처리를 할 수 있습니다. 저의 경우에는 이미 입력값이 command에서 진행되었기에 따로 새로 만든 객체에서 예외를 하지는 않았습니다.
src/main/java/domain/Board.java
Outdated
| private boolean isSameColor(final Location preLocation, final Location moveLocation) { | ||
| Pieces piecesInPreIndex = board.get(preLocation); | ||
| Pieces piecesInMoveIndex = board.get(moveLocation); | ||
| return piecesInPreIndex.isBlack() == piecesInMoveIndex.isBlack(); |
There was a problem hiding this comment.
- 이 부분을 여기서 해줘야할까?
- 현재 구조라면 같은 기물을 한번 더 움직일 수 있는데 개선해보는 것은 어떨까?
src/main/java/domain/Board.java
Outdated
| } | ||
| } | ||
|
|
||
| private void move(final Location preLocation, final Location moveLocation) { |
There was a problem hiding this comment.
이 부분에서 몇 가지 예외가 필요할 것 같아
조금만 더 고민해보는 것은 어떨까?
There was a problem hiding this comment.
이 부분에서 몇 가지 예외가 필요할 것 같아 조금만 더 고민해보는 것은 어떨까?
현재 로직은 이동전 기물의 위치에서 기물을 제거하고 이동하는 곳으로 기물을 덮어씌우는 로직입니다. 이때 발생할 수 있는 예외는 이동전 기물이 존재하는지, 이동동선에 기물이 있는지, 이동할 곳에 같은 기물이 있는지, 기물의 움직일 수 있는 위치인지와 같은 예외는 이미 처리했기습니다. 혹시 어떠한 예외처리가 필요한지 궁금합니다.
src/main/java/Main.java
Outdated
| @@ -1,2 +1,8 @@ | |||
| import controller.Controller; | |||
|
|
|||
| public class Main { | |||
src/main/java/domain/Knight.java
Outdated
| @@ -0,0 +1,36 @@ | |||
| package domain; | |||
|
|
|||
| import static domain.Direction.*; | |||
src/main/java/service/InitBoard.java
Outdated
|
|
||
| private void initPawn(final Map<Location, Pieces> board) { | ||
| for (int rank = RANK_INIT; rank < RANK_MAX; rank++) { | ||
| board.put(Location.from((char) (A + rank), BLACK_PAWN_FILE), new Pawn(State.BLACK_PAWN)); |
There was a problem hiding this comment.
A라고 표시한다면 대문자로 오인할 것 같은데 네이밍 변경을 해보면 어떨까?
현재 제 구조에서 enum State의 경우 이름에도 의미를 부여하고싶었습니다. 따라서 BLACK_PAWN 이런식으로 지었고, "P"와 "black"을 부여하였습니다. 다시 생각해보니 이는 결국 두 가지를 한번에 관리하는 느낌이 들었습니다. 따라서 이름자체에는 큰의미가 없더라도 Class Name으로 빼고 color의 경우에도 Class Team이런식으로 분리하는게 더 나을것 같다는 생각이 문득 들었습니다. 공통적인 부분이라 enum을 사용하여 의미를 부여하였지만 객체 관리의 입장과 도메인의 원자성을 고려하면 후자의 방법을 사용하는 것이 더 낫다고 생각하나요? |
|
사실 이정도면 충분한 것 같아서 머지를 시켰지만, 현재 구조에서 State에서 [기물, 컬러]로 구별하고, 마지막에 [컬러] 필드도 따로 주고 있습니다. 이렇게 해도 문제는 없을 것 같습니다. 지금 구조에서는 한 방법이 최선이라고 생각합니다. |
체스 리뷰 부탁드립니다!