[1, 2단계 - 체스] 토리(천은정) 미션 제출합니다. #498

ezzanzzan · 2023-03-17T08:02:12Z

안녕하세요 코즈 !
체스 미션을 함께 하게 된 토리입니다 ˙ᵕ˙

리팩토링 과정 중 궁금한 점을 아래에 함께 첨부합니다 ˙ᵕ˙
리뷰 잘 부탁드립니다 🙇‍♀️

이중 for문 대신 flatMap 사용

private List<Square> generateSquares() {
    List<Square> squares = new ArrayList<>();
    for (Rank rank:Rank.values()) {
        for (File file : File.values()) {
            squares.add(new Square(file, rank));
        }
    }
    return squares;
}

private List<Square> generateSquares() {
    return Arrays.stream(Rank.values())
            .flatMap(rank -> Arrays.stream(File.values())
                    .map(file -> new Square(file, rank)))
            .collect(Collectors.toList());
}

Squares를 생성할 때의 코드입니다.
stream 사용법에 익숙해지고 싶기도 했고, 이중 for문으로 인해 발생하는 2개 depth가 요구사항에 위배되어 flatMap을 통해 구현하게 되었는데 여기서 flatMap의 depth 2개로 봐야하는지를 여쭤보고 싶었습니다 !

저는 위의 첫 번째 코드와 두 번째 코드를 비교한다면, 이중 for문이 가독성이 더 좋다고 생각했습니다.
다만 이중 for문에서 depth를 줄이기 위해 메소드 분리를 하게 되면 flatMap이 더 가독성이 좋다고 생각해 현재 flatMap을 통해 코드를 구현하였는데 이 부분에 대한 코즈의 의견이 궁금합니다 🤔

인스턴스 변수를 바로 반환하는 메서드 네이밍에는 get이 없는 것에 대해 어떻게 생각하시나요?

private final int x;

File(final int x) {
    this.x = x;
}

public int x() {
    return x;
}

클래스에서 값을 반환받는 메소드들의 네이밍을 get~으로 통일해서 사용하고 있는데,
위 코드처럼 클래스의 인스턴스 변수를 바로 반환하는 메서드 네이밍에는 get 대신 인스턴스 필드의 네이밍을 그대로 사용하는 것에 대해 코즈의 의견이 궁금합니다 🤔

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

- Game 객체 제거 - Board 인스턴스 변수 자료형 변경 - Row 객체 제거 - Square piece 필드 제거 Co-authored-by: ezzanzzan <eunzzann@gmail.com>

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

- 타겟 위치에 상대팀 말이 존재할 경우 이동 가능 Co-authored-by: ezzanzzan <eunzzann@gmail.com>

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

kouz95

안녕하세요 토리 :)
리뷰 확인해주세요

for문과 flatMap 둘 다 괜찮다고 생각합니다 :)
같이 작업하는 사람과 의견이 맞춰질 수 있다면 상관 없다고 생각합니다. 토리가 생각하는 방식은 자바 14에서의 record 클래스에서 사용하고 있기도 합니다.

kouz95 · 2023-03-18T15:15:12Z

src/main/java/chess/GameCommand.java

+import java.util.List;
+
+public class GameCommand {
+    private enum gameCommandIndex {


Suggested change

private enum gameCommandIndex {

private enum GameCommandIndex {

kouz95 · 2023-03-18T15:16:15Z

src/main/java/chess/GameCommand.java

+        }
+    }
+
+    private enum moveIndex {


Suggested change

private enum moveIndex {

private enum MoveIndex {

클래스이기 때문에 대문자로 시작해야할 것 같아요.

자바 네이밍 규칙에 대해서 다시 한 번 정리하고 전체적으로 수정하였습니다 ! 🥺

kouz95 · 2023-03-18T15:17:00Z

src/main/java/chess/GameCommand.java

+    }
+
+    private void validateMoveCommand(final String source, final String target) {
+        if (!source.matches(SQUARE_BOUND_REGULAR_EXPRESSION) && target.matches(SQUARE_BOUND_REGULAR_EXPRESSION)) {


해당 로직은 도메인 로직으로 봐야하지 않을까요? 다른 view가 생긴다면 중복으로 적힐코드이지 않나요?

1단계 미션을 구현 할 때 start, end 만을 체크하는 기능이었는데, 2단계 미션을 구현하며 여러 기능이 추가되었던 걸 생각하지 못하고 파일 위치를 확인하지 못한 것 같습니다. 도메인 패키지로 이동시켰습니다 !

src/main/java/chess/GameCommand.java

kouz95 · 2023-03-18T15:22:40Z

src/main/java/chess/controller/ChessController.java

+        try {
+            return new GameCommand(inputView.readGameCommand());
+        } catch (final IllegalArgumentException e) {
+            System.err.println("[ERROR] " + e.getMessage());


위 문구는 체스 어플리케이션을 이용하는 사용자에게 노출하기 위한 문구일까요?
OutputView를 사용하지 않고 Controller에서 출력하는 이유가 있나요?

네! 체스 어플리케이션을 이용하는 사용자에게 보여주고 재입력을 받기 위한 메세지 입니다.
생각해보니 에러메시지를 출력하는 기능도 OutputView에 메서드로 만들어 볼 수 있겠네요 !!

kouz95 · 2023-03-18T15:36:55Z

src/main/java/chess/domain/board/Square.java

+    }
+
+    private boolean isMovableToTargetFile(final int targetFile, final int fileWeight) {
+        return getFile() + fileWeight == targetFile;


멤버변수에 직접 접근하는것이 아닌, getter를 사용하는 이유가 있나요?

public int getFile() { return file.getX(); } public int getRank() { return rank.getY(); }

return file.getX() + fileWeight == targetFile;

멤버 변수에 직접접근하게 되면 File 객체 내부의 값을 빼와야 하는데, 이 기능을 하는 메서드가 있어서 getter를 사용하여 코드를 구현하게 되었습니다 !

kouz95 · 2023-03-18T15:37:39Z

src/main/java/chess/domain/piece/King.java

+import java.util.List;
+
+public class King extends Piece {
+    private static final List<Move> possibleMoves = makePossibleMove();


Suggested change

private static final List<Move> possibleMoves = makePossibleMove();

private static final List<Move> POSSIBLE_MOVES = makePossibleMove();

kouz95 · 2023-03-18T15:40:15Z

src/main/java/chess/domain/piece/Piece.java

+    public boolean isMovable(final Square source, final Square target, final Move move) {
+        throw new UnsupportedOperationException();
+    }
+
+    public boolean isMovable(final Square source, final Square target, final KnightMove move) {
+        throw new UnsupportedOperationException();
+    }


KnightMove와 Move를 추상화하여 같은 타입으로 만들면, 추상메서드를 사용할 수 있을것 같습니다

KnightMove를 Move로 합치고 계산 로직을 변경하여 추상 메서드를 사용할 수 있도록 리팩토링 해보았습니다 !!

kouz95 · 2023-03-18T15:41:30Z

src/main/java/chess/domain/piece/Piece.java

+    }
+
+    @Override
+    public boolean equals(final Object o) {


체스판 위의 말들은 객체의 레퍼런스로 동일성 체크를 해도 괜찮지 않을까요? 어떻게 생각하시나요?

전체적인 코드에서 체스판 위의 말들을 체크할 때 Role과 Team을 사용해서 비교해주고 해당 역할에 맞는 기능을 수행하도록 구현하였는데 코멘트를 보고 equals 재정의가 된 기능을 사용한 부분을 다시 한 번 찾아보니 empty 객체를 비교할 때와, 테스트 코드에서만 사용되었다는 것을 확인했습니다.
equals를 통해 새로운 말을 생성해서 비교하는 것보다 piece에게 너 비어있는 말이니? 라고 물어보는 메서드를 추가하는 것이 좋아보여 isEmpty()라는 메서드로 비어있는 말을 체크하도록 리팩토링하였는데 의도하신 방향이 이것이 맞을까요 ?!

동일성과 동등성에 대해 참고해보셨으면 좋겠습니다 :)
https://creampuffy.tistory.com/140
Piece 객체가 속성이 같으면 같다고 봐야할 모델일 수도, 객체의 레퍼런스로 판단하는게 더 적절한 모델일 수도 있을텐데, 어떤걸 의도하고 equals 메서드를 정의하신건지 궁금하여 질문드린 것이었어요.

동일성과 동등성에 대해 이해하고 다시 질문을 읽어보니 의도하신 부분을 이해할 수 있었습니다 🥲
저는 Piece 내부의 값이 변하기도 하고, 객체의 레퍼런스로 판단하는 것보다 값을 비교해서 판단하는것이 좋다고 생각했습니다 !

kouz95 · 2023-03-18T15:42:27Z

src/main/java/chess/domain/piece/Role.java

+package chess.domain.piece;
+
+public enum Role {
+    KING("K"),


콘솔에 노출될 문구를 도메인이 갖고 있는것 같습니다. 추후 웹 view가 생기는 상황을 생각해보면, view가 도메인을 바라보고 어떻게 노출될지 결정하는 것이 확장성에 유리할 것 같아요

생각해보지 못했던 부분이었습니다 ㅠㅠ 오류 메시지 출력도 마찬가지였구요
좋은 코멘트 감사드립니다 🙇‍♀️

Role enum class에서 문구를 제거하고 view 패키지 안에 pieceMessage enum class를 생성하여 view에서 관리할 수 있도록 리팩토링해주었습니다 !!

- KnightMove enum class 삭제 - Piece class isMovable 메서드 추상 메서드로 변경

kouz95

현재 단계에서 요구하는 수준은 충분히 만족하셨다고 판단하여 머지합니다 :)
질문 주신 코멘트에도 답변 드렸으니 확인해주세요.
고생 많으셨습니다.

kouz95 · 2023-03-24T07:45:20Z

src/main/java/chess/domain/board/File.java

+
+    public static File findFile(int fileIndex) {
+        if (!FILES.containsKey(fileIndex)) {
+            throw new FileCanNotFindException();


현재 구조도 의존 방향을 생각해보면 domain -> exception -> 문구 의 구조가 되게 됩니다.
영어 버전을 지원하는 체스게임이 되어 영어 문구를 노출해야한다면 어떻게 해야할지 고민해보셨으면 좋겠습니다 :)

kouz95 · 2023-03-24T07:47:46Z

src/main/java/chess/domain/Game.java

+
+    public void move(final Square source, final Square target) {
+        if (board.isNotMyTurn(source, turn) || board.isEmptyPiece(source)) {
+            throw new IllegalArgumentException("자신의 말만 이동할 수 있습니다.");


리뷰 코멘트로 남겨드렸습니다 :)
다음 단계에서 적용해보시고 혹시 마땅한 방법이 떠오르지 않는다면 편하게 질문주세요

kouz95 · 2023-03-24T07:48:51Z

src/main/java/chess/domain/board/Move.java

+            return EMPTY;
+        }
+
+        return Arrays.stream(Move.values())


무조건 Map을 사용하는게 유리하다는 뜻은 아니었습니다. 토리가 생각하기에 가독성이 높아지거나, 딱히 성능적 이득을 볼 부분이 아니라고 판단이 된다면 순회도 좋습니다 :)

kouz95 · 2023-03-24T07:52:32Z

src/main/java/chess/domain/piece/Piece.java

+    }
+
+    @Override
+    public boolean equals(final Object o) {


동일성과 동등성에 대해 참고해보셨으면 좋겠습니다 :)
https://creampuffy.tistory.com/140
Piece 객체가 속성이 같으면 같다고 봐야할 모델일 수도, 객체의 레퍼런스로 판단하는게 더 적절한 모델일 수도 있을텐데, 어떤걸 의도하고 equals 메서드를 정의하신건지 궁금하여 질문드린 것이었어요.

hyeonjerry and others added 30 commits March 14, 2023 15:24

docs: 기능 요구사항 정리

b2a2667

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

docs: 제외 파일 추가

50c9e32

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: Piece(체스말) 생성 기능 구현

12aa890

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: Square(칸) 생성 기능 구현

13cbefa

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 빈 피스를 생성 기능 구현

be20463

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: Row(열) 생성 기능 구현

b454ab5

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: Board(판) 생성 기능 구현

f3219a1

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: Game 생성 기능 구현

74860c7

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

refactor: Role 빈 역할 initial 수정

f0114d5

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

refactor: Board 리팩토링

685a7b7

- Game 객체 제거 - Board 인스턴스 변수 자료형 변경 - Row 객체 제거 - Square piece 필드 제거 Co-authored-by: ezzanzzan <eunzzann@gmail.com>

docs: 기능 요구사항 업데이트

3c9ba19

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 체스판 초기화 기능 구현

dbc6696

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 게임 시작 커맨드 입력 및 출력 기능 구현

ef1ff6b

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

docs: 기능 요구사항 업데이트

9413c03

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 체스판 출력 기능 구현

1b8cf92

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

refactor: 랭크 enum 순서 변경

4305bce

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

docs: 2단계 기능 요구사항 작성

c63d198

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 폰 이동 기능 구현

031edf6

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 피스의 이동 경로 반환 기능 구현

d68ad29

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 이동 경로상에 피스가 존재할 경우 이동 제한 기능 구현

3926c8c

- 타겟 위치에 상대팀 말이 존재할 경우 이동 가능 Co-authored-by: ezzanzzan <eunzzann@gmail.com>

refactor: 게임 시작 커맨드 출력 내용 변경

68f692f

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 이동 가능 방향이 아닐 경우 이동 제한 기능 구현

aeae231

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

fix: 방향 판단 로직 수정

dbcb7c6

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

delete: 폰 객체 제거

1def780

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 킹 이동 기능 구현

e2ccfc9

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

fix: 킹의 한 칸 초과 이동 가능 오류 수정

95d4909

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 퀸 이동 기능 구현

189f451

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 룩 이동 기능 구현

eeb4d3f

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 비숍 이동 기능 구현

b5fbdc2

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 나이트 이동 기능 구현

bc17387

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

hyeonjerry and others added 8 commits March 16, 2023 15:12

style: 패키지 분리

b41bf07

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 폰 이동한 경우 두 칸 이동 못하게 하는 기능 구현

927ac65

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 체스 말 이동 기능 구현

c0a8e8a

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

style: 예외 메시지 수정

6140d88

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

docs: 요구 사항 업데이트

c60e11e

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 비어있는 칸 이동 제한 기능 구현

1cda808

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

feat: 턴제 기능 구현

4019fe1

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

refactor: 컨벤션 요구사항 리팩터링

859f8e5

Co-authored-by: ezzanzzan <eunzzann@gmail.com>

kouz95 requested changes Mar 18, 2023

View reviewed changes

ezzanzzan added 19 commits March 22, 2023 11:00

docs: 리팩토링 목록 정의

2dd6a98

chore: 패키지 변경

fa2cdb6

refactor: 불필요한 메서드 제거 및 필드 네이밍 수정

9e7abe4

refactor: 인터페이스에 대고 프로그래밍

541556c

feat: 예외 처리 기능 구현

e7abf10

feat: 오류 메세지 출력 기능 구현

5d48117

feat: 커스텀 예외 기능 구현

1e7dd7f

style: 클래스 네이밍 수정

690cb3e

style: 클래스 네이밍 수정

9f5cade

refactor: 비어있는 말 판단 기능 수정

a0beff3

refactor: 예외 처리 기능 추가

aee76f5

refactor: KnightMove 내부 기능 Move로 이동

369f0b0

- KnightMove enum class 삭제 - Piece class isMovable 메서드 추상 메서드로 변경

refactor: 메서드 네이밍 변경

4ca1580

refactor: 객체 네이밍 변경

055313b

feat: source와 target의 차이를 계산하는 기능 구현

3c58b61

feat: gcd로 방향을 계산하는 기능 구현

33471ad

test: 테스트 케이스 추가

63cc260

refactor: PieceMessage 캐싱 기능 구현

67a0280

refactor: 컨벤션 적용

e86e22e

kouz95 approved these changes Mar 24, 2023

View reviewed changes

kouz95 merged commit a3a1406 into woowacourse:ezzanzzan Mar 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1, 2단계 - 체스] 토리(천은정) 미션 제출합니다. #498

[1, 2단계 - 체스] 토리(천은정) 미션 제출합니다. #498

ezzanzzan commented Mar 17, 2023

kouz95 left a comment

kouz95 Mar 18, 2023

kouz95 Mar 18, 2023

ezzanzzan Mar 21, 2023

kouz95 Mar 18, 2023

ezzanzzan Mar 21, 2023 •

edited

Loading

kouz95 Mar 18, 2023

ezzanzzan Mar 21, 2023

kouz95 Mar 18, 2023

ezzanzzan Mar 21, 2023

kouz95 Mar 18, 2023

kouz95 Mar 18, 2023

ezzanzzan Mar 22, 2023

kouz95 Mar 18, 2023

ezzanzzan Mar 22, 2023

kouz95 Mar 24, 2023

ezzanzzan Mar 30, 2023

kouz95 Mar 18, 2023

ezzanzzan Mar 22, 2023

kouz95 left a comment

kouz95 Mar 24, 2023

kouz95 Mar 24, 2023

kouz95 Mar 24, 2023

kouz95 Mar 24, 2023

	private enum gameCommandIndex {
	private enum GameCommandIndex {

	private static final List<Move> possibleMoves = makePossibleMove();
	private static final List<Move> POSSIBLE_MOVES = makePossibleMove();

[1, 2단계 - 체스] 토리(천은정) 미션 제출합니다. #498

[1, 2단계 - 체스] 토리(천은정) 미션 제출합니다. #498

Conversation

ezzanzzan commented Mar 17, 2023

kouz95 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

ezzanzzan Mar 21, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kouz95 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

ezzanzzan Mar 21, 2023 •

edited

Loading