[Kyu] 미션 4: 모든 기물 배치하기 #130

kyupid · 2021-02-22T05:34:24Z

안녕하세요. 먼저 리뷰 진행 해주시는 분에게 감사드립니다. 미션 진행하면서 간략하게 느낀점? 작성해봤습니다.

1. 개념 이해

루카스에 있는 미션 요구사항과 안내사항을 보면서 처음 부딪친 것은 개념에 대한 이해였습니다. _Value Object_와 _static factory method_가 무엇인지 몰라서 공부하는 데에 시간을 하루 이상 썼습니다

2. StringUtils

완성된 보드를 출력하기 위해서 기존 로직에는 개행하는 문자열 \n 을 직접 삽입했었습니다. _StringUtils_라는 클래스를 따로 빼서 메서드에 문자를 삽입하는 식으로 방법을 바꾸었습니다. 루카스에서 안내한 대로 따라오긴 했지만, 어떤 이점이 있을까 생각해봤습니다.

\n 를 넣었을 땐 코드가 지저분해보였지만 쪼금? 더 깔끔하게 변했다 (가독성 향상)
정적 메서드는 이름을 가지고 있어서 _StringBuilder_에 무엇이 들어가는 건지 좀 더 한눈에 파악할 수 있다. (가독성 향상)
_StringBuilder_에 한 라인씩 깔끔하게 _append_할 수 있다. (가독성 향상)
직접 _\n_을 타입해서 넣는 것은 실수가 생길수도 있지만 정적 메서드를 사용하면서 그 실수를 줄일 수 있다. 왜냐하면 매개변수가 없으면 컴파일이 안되기 때문이다. (실수 발생 감소)

3. enum

꽤 많은 상수들이 생기기 시작해서 _enum_으로 처리했습니다. 근데 제 생각에는 지금은 그냥 상수를 나열해도 괜찮을 거 같습니다. 저는 _enum_을 사용하기가 왠지 모르게 꺼려지는 건 클래스 안에 클래스가 생기기 때문입니다. 코드가 별로 안이뻐보여서요.

4. main method

메인 메서드의 코드를 재작성했습니다. 이유는 조건문이 지저분해보여서였습니다. while문 안에서 입력을 받으니까 원하는 조건에 대한 처리가 훨씬 쉬워졌습니다.

감사합니다.

… in Board.print() - Board.print() 의 개행문자의 반복을 줄이기 위해서 StringUtils 라는 클래스를 따로 만들었습니다.

- System.getProperty는 사용해본 적이 없고 appendNewLine은 의도한대로 잘 작동하는지 확인하기위해서 테스트 코드를 작성했습니다.

- Pawn 으로 코드를 만들었지만 이제 다른 체스말들도 생성할 수 있도록 이름을 전면 수정

- Pawn 클래스가 Piece 클래스로 교체되었습니다. 교체되면서 각 체스말들을 생성하는 방법으로 Piece를 직접 생성하는게 아니라 정적 팩토리 메소드를 이용합니다.

- REPRESENTATION이 체스말마다 새로 생겨야하므로 기존에 있던건 PAWN으로 이름을 바꿨습니다.

- 기본생성자를 private 으로 변경했고, 기본 생성자를 외부, 내부에서 사용할 일이 없을 것 같아 코드를 제거했습니다

- 생성이 필요한 체스말들을 모두 상수로 넣었습니다.

- 길게 나열한 상수들을 enum에 담았습니다. - 다른 곳에서 enum들을 불러와서 사용할 때 모든 정보가 포함되어 있어서 가독성이 향상된 것 같습니다. 예를 들어서, Piece.White.PAWN.representation 처럼 모든 정보가 있어서 보기좋습니다. - 기존에 있던 상수들이 삭제되고 enum 으로 옮기면서 모두 이름을 바꿨습니다.

- 각 체스말을 만드는 정적 팩토리 메소드들을 만들었습니다.

- throw 를 던져주지 않아도 됩니다

- Pawn뿐만아니라 다른 체스말들도 체스판에 추가하여 출력이 잘되는지 확인하기위한 테스트를 구현하였습니다

- 이름을 더 명확하게 하기 위해서 showBoard로 바꿨습니다.

- 본래 리턴타입이 없었으나 테스트를 위해서 바꿨습니다. - 메인메소드에서 꺼내쓸때도 메인메소드에 직접 프린트api를 써주면 더 보기좋을거 같기도합니다

- 새로 만든 테스트 메소드와 겹치므로 삭제한다.

- 각 테스트 메소드에 새로 board를 초기화하는 것보다 `@BeforeEach` 를 사용해서 초기화하는 게 더 낫다고 생각

- Pawn 외에 다른 체스말들을 추가하기 위해서 새로운 List를 선언함 - initalize()에서 아까 추가한 새로운 List들에 Pawn외에 다른 체스말들을 초기화

…des Pawns - 이전에 추가했던 static method 인 appendNewLine을 이용해서 중복되는 ("\n") 을 제거합니다 - 구조를 짜기 위해서 현재는 없지만 만들어질 메서드를 미리 네이밍하여 showBoard()에 추가합니다. (getBlackPiecesResult(), getWhitePiecesResult())

- Pawn 뿐만아니라 Pieces 도 같이 사용하기 위해서 이름을 변경했습니다

- Pawn 뿐만아니라 나머지 piece들도 한 라인에 담아서 똑같이 showBoard()에서 나타내도록 했습니다

- 모든 체스말들이 32개인지 체크하기위해서 메소드를 추가합니다.

- 흰,검 구분을 위한 테스트 추가

- 테스트에 추가했던 코드들을 실제로 프로덕트 코드에 구현

- 가독성을 위해 코드를 정리합니다

- end 조건문이 중복이라서 수정했습니다

pbg0205 · 2021-02-22T08:20:15Z

src/main/java/net/honux/chess/pieces/Piece.java

@@ -9,6 +9,7 @@
        BISHOP('b'), QUEEN('q'), KING('k');


White, Black enum에서 코드 중복이 있는 부분을 색상(Color)하고 Type(혹은 Representation)을 각각 Enum으로 관리해보시는 방법도 고려해보시면 좋을거 같아요!

이왕 enum을 쓸거라면 쿠퍼말이 맞는거같아요,,제가 작성한건 그냥 상수로 쭉나열한거랑 다를바가 없다는 느낌이드네요. 감사합니다

pbg0205 · 2021-02-22T08:21:12Z

src/main/java/net/honux/chess/Main.java

-        }
-        if (game.equals("end")) {
-            System.exit(0);
+        while (!(game = sc.nextLine()).equals("end")) {


오! 저도 처음에 이 방법 생각했었는데ㅋㅋㅋ

전 요게 좋은거 같더라구요 ㅋㅋㅋ

오 이거 상당히 깔끔해보이네요

pbg0205 · 2021-02-22T08:24:21Z

kyu가 아까 놀러와줘서 저도 놀러왔습니다ㅋㅋㅋ 몇개 안되는데 코드 보면서 떠오르는 것(?)들 한번 적어봤어요! 코코아 같이 시작했을 때에 뵙던 때보다 훨씬 성장하신거 같아 자극받고 갑니다.👍👍 열정맨 kyu 화이팅 (리뷰할 때 코드 범위 지정(?) 하는 법을 몰라서 우당탕 되어있는데 그 부분은 양해 부탁해요ㅎㅎ)

src/main/java/net/honux/chess/pieces/Piece.java

kyupid

아하 그렇군요 boolean 으로 리턴했을때 그냥 조건만 주면 알아서 판단해서 true, false를 반환하는거였군요

kyupid · 2021-02-22T08:36:57Z

kyu가 아까 놀러와줘서 저도 놀러왔습니다ㅋㅋㅋ 몇개 안되는데 코드 보면서 떠오르는 것(?)들 한번 적어봤어요! 코코아 같이 시작했을 때에 뵙던 때보다 훨씬 성장하신거 같아 자극받고 갑니다.👍👍 열정맨 kyu 화이팅 (리뷰할 때 코드 범위 지정(?) 하는 법을 몰라서 우당탕 되어있는데 그 부분은 양해 부탁해요ㅎㅎ)

오 ㅎㅎ 시작했을때보다 많이 성장했어요?? 그 말 정말 기쁘네요

SANGCO

Kyu님 구현 잘하셨네요. 리뷰 드릴게 별로 없네요ㅋ
간단한 리뷰 남겼으니 확인해 보시고 질문 있으시면 댓글 부탁드립니다~

SANGCO · 2021-02-22T12:20:21Z

src/main/java/net/honux/chess/pieces/Piece.java

+    public enum White {
+        PAWN('p'), KNIGHT('n'), ROOK('r'),
+        BISHOP('b'), QUEEN('q'), KING('k');
+
+        public char representation;
+
+        White(char representation) {
+            this.representation = representation;
+        }
+    }
+
+    public enum Black {
+        PAWN('P'), KNIGHT('N'), ROOK('R'),
+        BISHOP('B'), QUEEN('Q'), KING('K');
+
+        public char representation;
+
+        Black(char representation) {
+            this.representation = representation;
+        }


White, Black 이렇게 색깔로 enum을 만드셨군요.
이번 미션을 수행하는데 있어서는 문제 될게 없을거 같습니다~

오오 이런 방식!! 깔끔하고 좋네요! 👍👍

SANGCO · 2021-02-22T12:25:04Z

src/main/java/net/honux/chess/Board.java

-    private ArrayList<Pawn> distinguishPawnsColor(Pawn pawn) {
-        if (pawn.getColor() == Pawn.WHITE_COLOR)
+    private ArrayList<Piece> distinguishPawnsColor(Piece pawn) {
+        if (pawn.getColor() == Piece.WHITE_COLOR)
            return whitePawns;
        else
            return blackPawns;
    }

    void initialize() {


이 메서드의 접근제한자를 디폴트로 하신 이유가 궁금합니다~

initialize() 말씀하시는 거죠? public 이었는데 지난 미션에서 실수로 지웠던거 같습니다

SANGCO · 2021-02-22T12:27:49Z

src/main/java/net/honux/chess/Main.java

+        while (!(game = sc.nextLine()).equals("end")) {
+            if(game.equals("start"))
+                System.out.println(board.showBoard());


"end", "start" 같은 스트링 값들을 상수로 만들어서 사용하는건 어떨까요?

제가 잘몰라서,, 그냥 스트링으로 "start" , "end" 로 적어주었을때랑 상수선언했을때와 별다른 차이점을 잘 모르겠습니다.

예를 들어서,for (int i = 0; i < 8; i++) { 체스말초기화하는 메서드들} 있을 때, 숫자 8은 누가봐도 무슨 의미인지 확인하는 데에 시간이 오래걸리기때문에 BOARD_SIZE 라는 상수로 선언해줬었는데요.

이번 경우에는 잘 모르겠습니다. 설명을 덧붙여주시면 고민해보겠습니다!

제가 생각하는 장점은
스트링 값을 상수로 만들어 변경 지점을 한군데로 하면
향후에 추가로 재사용이나 메소드 추출, 클래스 변수, enum 등으로 바꾸기 용이한거 같아요.

호눅스는 어떻게 생각하시나요?
제가 좀 과한 리뷰를 드린걸까요?ㅋ
아니면 또 다른 장점들이 있을까요~

SANGCO · 2021-02-22T12:28:22Z

src/main/java/net/honux/chess/Main.java

@@ -5,24 +5,18 @@
 public class Main {

    public static void main(String[] args) {
+        Scanner sc = new Scanner(System.in);


Scanner와 같이 AutoCloseable을 구현한 클래스의 경우
try-with-resources를 사용해서 쉽게 리소스를 해제할 수 있답니다.
이번 미션에서 try-with-resources 관련되어서 학습하고 적용해보면 좋을거 같아요~

넵 감사합니다. 공부해보고 적용해보겠습니다!

공부해서 적용했습니다. Scanner scanner = new Scanner(System.in) 같이 예외처리 안해줘도 되는건 try catch 문에서 try만 작성해줘도 되는것이죠? IDE에서는 아무 에러도 안나고 잘 작동해서 질문드립니다.

try-with-resources는 단순히 try()문 괄호 안에 넣은 AutoCloseable을 구현한 클래스에 close()를 호출해 주는거고
만약 try()로 감싼 로직에 익셉션을 던지는 부분이 있으면
catch로 잡던지 밖으로 던지던지 해야할거 같군요!

아하 이해했습니다. 감사합니다! 😊

- 딱히 default로 선언할 이유가 없다

- try-with-resources 를 사용해서 자원이 자동으로 닫히도록 만들었습니다

SANGCO

고생 많으셨습니다~

kyupid added 24 commits February 18, 2021 10:12

Implement class StringUtiils to avoid duplicated code making new line…

2379fd8

… in Board.print() - Board.print() 의 개행문자의 반복을 줄이기 위해서 StringUtils 라는 클래스를 따로 만들었습니다.

Verify class StringUtils

b473120

- System.getProperty는 사용해본 적이 없고 appendNewLine은 의도한대로 잘 작동하는지 확인하기위해서 테스트 코드를 작성했습니다.

Refactor the name of Pawn to Piece

e2e5edb

- Pawn 으로 코드를 만들었지만 이제 다른 체스말들도 생성할 수 있도록 이름을 전면 수정

Verify static factory method

b23b8af

- Pawn 클래스가 Piece 클래스로 교체되었습니다. 교체되면서 각 체스말들을 생성하는 방법으로 Piece를 직접 생성하는게 아니라 정적 팩토리 메소드를 이용합니다.

Rename constant REPRESENTATION

fa116d9

- REPRESENTATION이 체스말마다 새로 생겨야하므로 기존에 있던건 PAWN으로 이름을 바꿨습니다.

Remove unnecessary construct code and test code for it as well.

af95813

- 기본생성자를 private 으로 변경했고, 기본 생성자를 외부, 내부에서 사용할 일이 없을 것 같아 코드를 제거했습니다

Add a bunch of constants of chess pieces

8380bf8

- 생성이 필요한 체스말들을 모두 상수로 넣었습니다.

Add a bunch of methods creating certain a piece object

c2f22bd

- 각 체스말을 만드는 정적 팩토리 메소드들을 만들었습니다.

Remove useless throw

dda59c1

- throw 를 던져주지 않아도 됩니다

Verify a chess board initialized

f9ef32b

- Pawn뿐만아니라 다른 체스말들도 체스판에 추가하여 출력이 잘되는지 확인하기위한 테스트를 구현하였습니다

Rename print() in Board to showBoard

6e4d894

- 이름을 더 명확하게 하기 위해서 showBoard로 바꿨습니다.

Fix return type of showBoard() in Board

99f97f1

- 본래 리턴타입이 없었으나 테스트를 위해서 바꿨습니다. - 메인메소드에서 꺼내쓸때도 메인메소드에 직접 프린트api를 써주면 더 보기좋을거 같기도합니다

Remove duplicated Test

14272d1

- 새로 만든 테스트 메소드와 겹치므로 삭제한다.

Fix initialization in Test class

b602948

- 각 테스트 메소드에 새로 board를 초기화하는 것보다 `@BeforeEach` 를 사용해서 초기화하는 게 더 낫다고 생각

Add new list for other pieces besides

20b01af

- Pawn 외에 다른 체스말들을 추가하기 위해서 새로운 List를 선언함 - initalize()에서 아까 추가한 새로운 List들에 Pawn외에 다른 체스말들을 초기화

Fix parameter's name of listPiecesToSingleLine

aa9569f

- Pawn 뿐만아니라 Pieces 도 같이 사용하기 위해서 이름을 변경했습니다

Add PiecesResult() for showBoard()

7533da5

- Pawn 뿐만아니라 나머지 piece들도 한 라인에 담아서 똑같이 showBoard()에서 나타내도록 했습니다

Add pieceCount()

a860a29

- 모든 체스말들이 32개인지 체크하기위해서 메소드를 추가합니다.

Add a test that tells if the piece is white or black

7360bff

- 흰,검 구분을 위한 테스트 추가

Add a method that tells if the piece is white or black

c9f4f5c

- 테스트에 추가했던 코드들을 실제로 프로덕트 코드에 구현

Arrange blank line

3adf540

- 가독성을 위해 코드를 정리합니다

Refactor main method

ef1efa6

- end 조건문이 중복이라서 수정했습니다

pbg0205 reviewed Feb 22, 2021

View reviewed changes

src/main/java/net/honux/chess/pieces/Piece.java Show resolved Hide resolved

kyupid commented Feb 22, 2021

View reviewed changes

SANGCO self-requested a review February 22, 2021 12:10

SANGCO self-assigned this Feb 22, 2021

SANGCO requested changes Feb 22, 2021

View reviewed changes

kyupid added 2 commits February 23, 2021 09:40

Fix access modifier of initialize()

6e0ade8

- 딱히 default로 선언할 이유가 없다

Use try-with-resources for scanner in main method

27db1b9

- try-with-resources 를 사용해서 자원이 자동으로 닫히도록 만들었습니다

kyupid requested a review from SANGCO February 23, 2021 01:08

SANGCO approved these changes Feb 23, 2021

View reviewed changes

honux77 merged commit 21a98b0 into codesquad-members-2021:kyu-kim-kr Feb 24, 2021

kyupid deleted the step4 branch July 5, 2021 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Kyu] 미션 4: 모든 기물 배치하기 #130

[Kyu] 미션 4: 모든 기물 배치하기 #130

kyupid commented Feb 22, 2021

pbg0205 Feb 22, 2021 •

edited

kyupid Feb 22, 2021

pbg0205 Feb 22, 2021

kyupid Feb 22, 2021

tmdgusya Feb 23, 2021

pbg0205 commented Feb 22, 2021 •

edited

kyupid left a comment

kyupid commented Feb 22, 2021

SANGCO left a comment

SANGCO Feb 22, 2021

jihye-woo Feb 23, 2021

SANGCO Feb 22, 2021

kyupid Feb 22, 2021

SANGCO Feb 22, 2021

kyupid Feb 22, 2021

SANGCO Feb 23, 2021

SANGCO Feb 22, 2021

kyupid Feb 22, 2021

kyupid Feb 22, 2021

SANGCO Feb 23, 2021

kyupid Feb 23, 2021

SANGCO left a comment

[Kyu] 미션 4: 모든 기물 배치하기 #130

[Kyu] 미션 4: 모든 기물 배치하기 #130

Conversation

kyupid commented Feb 22, 2021

1. 개념 이해

2. StringUtils

3. enum

4. main method

pbg0205 Feb 22, 2021 • edited

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

pbg0205 commented Feb 22, 2021 • edited

kyupid left a comment

Choose a reason for hiding this comment

kyupid commented Feb 22, 2021

SANGCO 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

Choose a reason for hiding this comment

SANGCO left a comment

Choose a reason for hiding this comment

pbg0205 Feb 22, 2021 •

edited

pbg0205 commented Feb 22, 2021 •

edited