[1, 2, 3단계 - 체스] 기론(김규철) 미션 제출합니다. #295

Gyuchool · 2022-03-28T06:01:27Z

안녕하세요 토니!
기론이라고 합니다!! 😎 이번 체스미션은 블랙잭보다 난이도가 많~이 뛰어서 체감상 어려웠습니다.😂😂
아직 아쉬운 코드들이 있는데 어떻게 해야 더 클린한 코드가 될지 고민이네요.
이번 체스 미션 리뷰 잘 부탁 드립니다!

감사합니다.

toneyparky

안녕하세요 기론!

체스 게임의 리뷰어 토니에요.
작성해주신 코드 잘 확인했어요. 꾸준하게 리팩터링하여 발전하려는 모습이 눈에 보였습니다!

몇가지 리뷰를 남겼으니 확인하고 수정하여 더 이해하기 좋은 코드로 발전시켜보죠!
질문에 대해 대댓글 남겨주시구 DM 은 언제든 환영이에요!

화이팅입니다!

toneyparky · 2022-03-29T11:48:03Z

docker/docker-compose.yml

@@ -0,0 +1,18 @@
+version: "3.9"


docker-compose는 어떤 의도에서 추가하셨는지 궁금해요~!

아 docker-compose는 오늘 수업 중에 실습 했던 내용입니다!
깃에 추가하고 싶은 사람은 추가해도 된다고 해서 추가했었습니다!

오호 그런 배경이 있었군요!

해당 기술을 잘 익혀두시면 추후에 프로젝트할 때나 현업에 와서도 잘 활용하실 수 있을거에요!

toneyparky · 2022-03-29T11:49:55Z

src/main/java/chess/Board.java

+        if (canMove(targetPosition, sourcePiece, targetPiece)) {
+            move(sourcePosition, targetPosition, sourcePiece, targetPiece);
+            return;
+        }
+        throw new IllegalArgumentException("움직일수 없습니다.");


if문 내에서 예외를 던지를 처리를하고 그렇지 않을 경우에 그냥 리턴을 하도록 바꾼다면 지금과 어떤 차이가 있을까요?

if문 내에서 예외를 던지도록 하면 validate와 같이 메서드로 뺄수 있겠군요!
또한 if문에 걸려서 예외가 발생하면 어떤 상황에서의 예외인지 알 수 있어서 디버깅할때도 좋을 것 같습니다!

좋네요! 코드도 확인했는데 훨씬 더 깔끔해졌어요!

toneyparky · 2022-03-29T11:54:42Z

src/main/java/chess/command/Init.java

+        if (input.contains("move")) {
+            throw new IllegalArgumentException("시작 명령어에는 move가 들어갈수 없습니다.");
+        }
+        if ("start".equals(input)) {
+            return new Start(input);
+        }
+        if ("end".equals(input)) {
+            return new End(input);
+        }
+        throw new IllegalArgumentException("command has only move or end ");
+    }


예외 메시지에 영어와 한글을 섞어서 사용하신 이유가 궁금해요 😁

큰 이유는 없어요...ㅎㅎ😅
페어와 진행하면서 여기에 예외 던지면 의견이 나왔었고 에러 메시지로는 생각나는대로 적어둔 것이었습니다!
통일성을 위해 한글로 수정할게요!

깔끔하게 수정해주셨네요! 굳!

toneyparky · 2022-03-29T11:55:24Z

src/main/java/chess/command/Command.java

+
+    @Override
+    public List<String> getCommandPosition() {
+        throw new IllegalArgumentException("명령어에서 위치를 얻을수 없습니다.");


IllegalArgumentException는 어떤 경우에 발생하는 예외일까요?

지금 상황을 보다 적절하게 표현할 수 있는 예외가 있을까요~!?

이펙티브 자바에 의하면

IllegalArgumentException은 허용하지 않는 인수가 들어오는 경우

IllegalStateException은 객체가 메서드를 수행하기에 적절하지 않은 상태인 경우

UnsupportedOpertionException 은 호출한 메서드를 지원하지 않을 때 사용한다고 하네요.

IllegalStateException와 UnsupportedOpertionException 중 어느 것이 더 적합할지 고민이었는데 아래 stackoverflow글의 도움을 받아서 현재는 재정의된 상태이므로 IllegalStateException을 사용하도록 수정하겠습니다!

IllegalStateException vs UnsupportedOpertionException

좋아요 기론!

자바의 예외 체계는 잘 구성되어 있기에 적절한 예외를 선택해서 사용한다면 예외 상황을 마주했을 때에 좋은 힌트가 될거에요!

toneyparky · 2022-03-29T11:57:40Z

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

+import chess.view.OutputView;
+
+public class ChessController {
+    public void run() {


코드 리뷰를 하는 동안 해당 클래스의 길이가 줄어들었네요!

메서드 분리 적절하게 잘 하신거 같아요 👍

toneyparky · 2022-03-29T12:02:31Z

src/main/java/chess/view/OutputView.java

+    private static void makeNewLine(Piece piece) {
+        if (piece.isLastFile()) {
+            System.out.println();
+        }
+    }


Piece의 메서드를 사용하시군요! OutputView와 도메인이 결합되어 있어보이네요~!

dto 사용으로 결합을 끊었습니다!

넵~! 좋아요.

콘솔이 아닌 웹뷰를 적용하실텐데 도메인과의 연결을 끊는 연습을 했다고 생각합니다!

toneyparky · 2022-03-29T12:04:10Z

src/main/java/chess/piece/Pieces.java

+                .orElseThrow(() -> new IllegalArgumentException("해당 위치에 말이 없습니다."));
+    }
+
+    public long countOfKing() {


해당 메서드에 대한 테스트가 빠져있군요!
다른 클래스에도 기론이 생각하기에 필요한 테스트가 빠져 있다면 추가해보아요~!

toneyparky · 2022-03-29T12:07:18Z

src/test/java/chess/piece/PiecesTest.java

+    }
+
+    @Test
+    @DisplayName("체스판에 남은 말들의 점수를 계산한다. - 폰이 전부 가로로 있을 때")


이름만 봐서는 폰 이외의 다른 말에 대해서도 테스트한다는 내용을 파악하기가 조금 힘들었어요~!

기론이 테스트하고 싶은 내용에 포커스를 맞춰서 수정해보면 더 명확한 테스트가 될 거 같아요!

toneyparky · 2022-03-29T12:08:37Z

src/test/java/chess/CommandTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class CommandTest {


테스트 클래스는 프러덕션 코드의 클래스와 같은 패키지 선상에 위치해야합니다!

또한 내용은 Command보다는 Init과 End에 대해서 테스트하는 것으로 보여져요!

toneyparky · 2022-03-29T12:08:59Z

src/test/java/chess/piece/KingTest.java

+import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+
+class KingTest {


테스트를 꼼꼼하게 잘 작성해주셨네요! 좋습니다 👍

toneyparky · 2022-03-30T12:34:57Z

build.gradle

@@ -12,6 +12,7 @@ dependencies {
    implementation 'com.sparkjava:spark-core:2.9.3'
    implementation 'com.sparkjava:spark-template-handlebars:2.7.1'
    implementation 'ch.qos.logback:logback-classic:1.2.10'
+    runtimeOnly 'mysql:mysql-connector-java:8.0.28'


DB가 붙는군요! 설레네요(?)

여기서 한가지 퀴즈~!

runtimeOnly와 implementation 그리고 testImplementation는 각각 어떤 의미로 build.gradle에서 사용될까요? 댓글로 남겨주세요!

오 퀴즈는 언제나 설레네요 ㅎㅎㅎ
퀴즈 공부하면서 grdle의 의존성 옵션에 대해서 알아볼 수 있었습니다!
runtimeOnly

runtimeClassPath에만 추가된다.

해당 클래스에서 코드 변경이 발생해도 런타임에만 불러오기 때문에 컴파일을 다시 할 필요가 없다는 장점이 있습니다.

implementation

A->B->C 모듈 순서로 의존성이 있을 때, A에서 B를 implementation으로 설정하면 B에 대한 모듈만 가져오고 C에 대해서는 가져오지 않습니다!

따라서 C가 수정되었더라도 A를 다시 컴파일할 필요가 없습니다.

testImplementation

implementation이 테스트코드를 수행할 때만 적용한다.

와 같이 정리했습니다!
정리글

정리 잘 해주셨네요! 좋습니당 블로그 글도 확인했어요 기론은 글도 잘 작성하시는군요 👍

toneyparky · 2022-03-30T13:32:14Z

src/main/java/chess/Board.java

+
+public final class Board {
+
+    private final Pieces pieces;


체스판을 구현하는 여러가지 방법이 있을텐데요. 기론은 자신의 위치에 해당하는 정보를 들고 있는 각 기물들의 모음을 체스판으로 봤어요.

다른 방식으로 구현한다면 8X8의 체스판 자체를 이중 리스트나 맵 형식으로 구조화할 수도 있겠죠!

기론이 기론의 방식으로 구현하며 느낀 장점이나 단점이 있을까요? 다른 방식으로 구현한다면 어떤 장단점이 있을지도 상상해보면 재미있겠네요!

처음엔 Map<Position, Piece>를 갖는 board를 만들었습니다. 그런데 구현을 진행하면서 페어와 board가 가지고 있는 상태에 대해서 이야기를 했는데, board에서 위치와 말을 모두 알아야하나? 라는 생각을 먼저 했습니다.

board가 piece를 움직여서 게임하는 것이므로 piece들만 관리하는 게 더 맞지 않을까 라고 생각하였고 board가 piece만 알고 piece가 자신의 위치를 가지고 있도록 구현하였습니다.

또한 board가 piece와 position모두를 관리하지 않고 piece들만 관리하면 되기 때문에 결합도가 더 낮아지지않을까? 라고 생각했습니다.

장점으로는 board가 pieces를 가지면서 board클래스 내에서 하는 일이 많이 줄어들었습니다. 대부분 piece들이 움직이므로 책임의 대부분이 pieces 또는 piece로 갔어요!
단점으로는 체스판을 출력해줄 때, piece의 위치를 매번 확인해서 마지막 file에 있는 자리인지 확인해야 한다는 단점이 있었습니다.

아마 기존의 Map방식으로 사용한다면
Map의 자료구조 특성상 요소의 추가 삭제가 List보다 성능이 나을 때가 많고 검색 성능은 더 좋아서 성능상에선 더 좋을 것 같네요🤔
대신 제 생각에는 객체의 행위에 집중하면 board가 position까지 알 필요가 있나 라고 생각되어서 이게 단점인 것 같습니다.

생각하면 생각할수록 어렵네요🤔🤔 각각 트레이드오프가 있는 것 같습니다.
토니였다면 어떤 선택을 하셨을까요?

또한 토니는 제 생각에 대해서 어떻게 생각하시나요?

객체의 행위에 집중하면 board가 position까지 알 필요가 있나?

board는 piece들을 가지고 게임하므로 position은 알 필요가 없다고 생각해서 나온 이 생각이 정말 객체의 행위에 집중한 게 맞을까요?
오브젝트 책의 내용에 나온 행동을 바탕으로 설계하는 것을 적용해보려고 하는데 읽는 것과 실제 적용하기에는 너무 어렵네요😂😂

크... 많은 고민을 하셨군요 기론! 상세하게 적어주셔서 이해하는 데에 도움이 많이 되었어요 감사합니다! 페어와 함께 고민하셨다는 부분 너무 좋네요.

항상 개발을 하다보면 트레이드 오프가 있죠! 체스 게임이 엄청 큰 서비스가 아니기에 성능적인 고려보다는 코드를 이해하기 좋은 방식으로 구성할거 같아요. 지금 기론의 방식은 좋은 접근이라 생각해요. 각 piece를 자신의 위치도 관리하는 주체적인 객체라고 정의한다!

toneyparky · 2022-03-30T13:33:23Z

src/main/java/chess/ConsoleApplication.java

+
+public class ConsoleApplication {
+    public static void main(String[] args) {
+        ChessController chessController = new ChessController();


현재의 구조에서는 게임을 진행하다가 실수할 경우에 체스 게임이 바로 종료되어요 😂

요구사항에 명시되지 않았지만 프러덕트를 개발하는 개발자로서 사용자가 실수하더라도 적절한 메시지를 노출한 후에 이어서 진행할 수 있도록 수정해보는건 어떨까요?

명짤(?)이네요.. 저장해둬야겠습니다😁수정했습니다!

toneyparky · 2022-03-30T13:54:43Z

docker/docker-compose.yml

@@ -0,0 +1,18 @@
+version: "3.9"


오호 그런 배경이 있었군요!

해당 기술을 잘 익혀두시면 추후에 프로젝트할 때나 현업에 와서도 잘 활용하실 수 있을거에요!

toneyparky

리뷰 반영 잘 해주셨어요 기론!

한가지 퀴즈(?)와 코멘트를 남겼으니 확인 후 반영 부탁드려요!

이야기 나누고 싶은 부분이 있다면 DM주셔도 좋아요~!

toneyparky · 2022-04-01T06:43:48Z

src/main/java/chess/piece/position/Position.java

@@ -15,8 +20,11 @@ private Position(File file, Rank rank) {
    public static Position of(char... position) {
        File file = File.valueOf(position[0] - ASCII_TO_INT);
        Rank rank = Rank.valueOf(Character.getNumericValue(position[1]));
+        return CACHE.computeIfAbsent(createKey(file, rank), absent -> new Position(file, rank));


요 메서드는 저도 배워가네요! 굳굳

toneyparky · 2022-04-01T06:44:51Z

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

+            command = command.turnState(input);
+            return command;
+        } catch (IllegalArgumentException e) {
+            System.err.println(e.getMessage());


에러에 대한 출력도 출력을 담당하는 객체가 수행하도록 수정해볼까요?

toneyparky · 2022-04-01T06:44:57Z

build.gradle

@@ -12,6 +12,7 @@ dependencies {
    implementation 'com.sparkjava:spark-core:2.9.3'
    implementation 'com.sparkjava:spark-template-handlebars:2.7.1'
    implementation 'ch.qos.logback:logback-classic:1.2.10'
+    runtimeOnly 'mysql:mysql-connector-java:8.0.28'


정리 잘 해주셨네요! 좋습니당 블로그 글도 확인했어요 기론은 글도 잘 작성하시는군요 👍

toneyparky

안녕하세요 기론 😊

제가 남긴 리뷰에 대해 답변을 잘 해주셨네요! 더 나아가 캐싱까지 적용하신 점도 인상깊었어요.
이번 단계는 머지해도 충분할거 같아요. 다음 단계로 나아가보죠!

고생 많으셨습니다.

Gyuchool added 30 commits March 22, 2022 15:43

docs: 1단계 기능 요구 사항 정리

6c8ca60

feat: 체스판을 만들고 체스 말들을 초기화 시킨다.

5fbb082

feat: start와 end가 아닌 입력이 들어오면 예외가 발생한다.

325f6ee

feat: 게임 시작과 종료 기능 구현

110c301

feat: 체스판과 말들을 출력한다.

742ff00

refactor: rank 순서 변경

9f5689e

docs: 2단계 기능 요구 사항 정리

10e5026

feat: position끼리 수평에 위치하는지 비교하는 기능 구현

23e2261

feat: position끼리 수직에 위치하는지 확인하는 기능 구현

27341e6

test: 룩의 진행방향이 다르면 예외가 발생하는 테스트 추가

4a094b2

refactor: piece의 추상메서드 수정

84c3914

feat: position이 대각선에 있는지 비교하는 기능 구현

649f8db

feat: 퀸의 진행방향이 옳은지 확인하는 기능 구현

78e60ce

feat: 킹의 진행방향과 거리가 옳은지 확인하는 기능 구현

0447917

feat: 나이트의 진행방향과 거리가 옳은지 확인하는 기능 구현

32e0cf8

feat: 폰의 진행방향과 거리가 옳은지 확인하는 기능 구현

62d79c0

refactor: piece들이 각각 이름을 갖도록 수정

cc548fe

feat: 체스판의 빈곳을 표현하는 말 추가

0f6128e

refactor: compareTo 메서드에서 depth 2 제거

4baa7d6

feat: command 구조 상태패턴으로 변경

95abedd

feat: 룩이 상하좌우로 칸수 제한없이 움직이도록 기능 구현

d44b7f2

feat: 킹이 상하좌우, 대각선 한 칸만 움직일수 있다.

e2b867a

feat: 비숍은 대각선으로만 칸수 제한없이 움직인다.

e9a9253

feat: 나이트의 이동 규칙에 맞게 기능 구현

61b3ce2

feat: 퀸이 상하좌우, 대각선 칸수 제한없이 움직인다.

754ff24

feat: 폰의 이동규칙에 맞게 기능 추가

708d657

feat: 폰이 상대 말을 잡을때의 기능 구현

aadcce9

test: 체스판 범위를 벗어나는지 검증 테스트

7bb3409

refactor: OutputView에서 게임 시작 메세지를 출력한다.

c235483

docs: 3단계 기능 요구사항 정리

7f92619

Gyuchool added 4 commits March 26, 2022 22:07

feat: 남아있는 말에 대한 점수를 계산한다.

0a4fead

feat: 게임이 끝나고 status를 입력하면 결과를 보여주고 우승 팀을 알려준다.

2cd9b8b

feat: 턴을 추가한다.

daa63dc

refactor: 안쓰는 import문 제거 및 코드 포멧팅

58212d7

Gyuchool changed the title ~~[1, 2, 3단계 - 체스] 루키(최지훈) 미션 제출합니다.~~ [1, 2, 3단계 - 체스] 기론(김규철) 미션 제출합니다. Mar 28, 2022

Gyuchool added 4 commits March 28, 2022 20:36

refactor: final class로 수정

1283e48

refactor: killstrategy 제거

5c6e0a8

refactor: stream을 이용하여 변경

afbbb7a

feat: docker-compose 설치

354e7bd

toneyparky requested changes Mar 29, 2022

View reviewed changes

Gyuchool added 11 commits March 29, 2022 23:21

refactor: error메시지 수정

8e2d464

refactor: 예외가 발생할 때 return 을 던짐으로써 validate메서드 분리

5a60cb1

refactor: 불필요한 주석 제거

7f111ef

refactor: 인터페이스 이름 범용적이도록 수정

40b2a1f

refactor: 정적 펙터리 메서드 네이밍 수정

0e074b5

refactor: 빈칸 초기화하는 메서드 네이밍 수정

4f377c2

refactor: view에서 도메인 의존성 제거

c0c53e8

test: pieces 테스트 추가

3190f12

test: command관련 테스트 추가

b7b3972

refactor: 변수 이름 의미 명확하도록 수정

ad6e2bb

refactor: 조건문 메서드로 분리

42df1b1

Gyuchool mentioned this pull request Mar 30, 2022

[체스 미션] 기론 1차 리뷰 정리 woowacourse-study/2022-back-end-code-review-study#24

Merged

toneyparky reviewed Mar 30, 2022

View reviewed changes

toneyparky requested changes Mar 30, 2022

View reviewed changes

Gyuchool added 3 commits March 31, 2022 01:51

refactor: 게임이 바로 종료되지 않도록 수정

8235b85

refactor: 명령어 입력 형식 잘못되어도 다시 입력받도록 수정

6c770d0

refactor: position에 캐싱 적용

6896212

toneyparky reviewed Apr 1, 2022

View reviewed changes

toneyparky approved these changes Apr 1, 2022

View reviewed changes

toneyparky merged commit 5c860cb into woowacourse:gyuchool Apr 1, 2022

[1, 2, 3단계 - 체스] 기론(김규철) 미션 제출합니다. #295

[1, 2, 3단계 - 체스] 기론(김규철) 미션 제출합니다. #295

Conversation

Gyuchool commented Mar 28, 2022

toneyparky 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

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

Gyuchool Mar 30, 2022 • 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

toneyparky 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

toneyparky left a comment

Choose a reason for hiding this comment

Gyuchool Mar 30, 2022 •

edited