-
Notifications
You must be signed in to change notification settings - Fork 416
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[3, 4단계 - 체스] 저문(유정훈) 미션 제출합니다. #627
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 저문! 다니입니다 🙌
이번 단계 어려웠을텐데 잘 구현해주셨어요 💯
코드리뷰 확인해주세요.
질문에 대한 답변은 별도 코멘트로 곧 남길게요!
혹시 궁금한 점이 있으시다면 언제든지 편하게 코멘트나 슬랙 DM 주세요 🙇♀️
README.md
Outdated
@@ -62,3 +61,24 @@ | |||
## 체스게임 | |||
|
|||
- [x] 게임 시작과 종료를 제어한다. | |||
|
|||
- [ ] status입력이 들어오면 현재의 각 진영의 점수를 출력하고, 이긴 진영을 출력한다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
체크 표시 놓치신 것 같아요 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
시간에 쫓겨 리뷰요청을 보내다보니 기능 목록 최신화가 우선순위에서 벗어났네요 ㅠㅠ
깔끔하게 수정해보겠습니다!
@Test | ||
public void connection() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -11,6 +11,7 @@ repositories { | |||
dependencies { | |||
testImplementation 'org.assertj:assertj-core:3.22.0' | |||
testImplementation 'org.junit.jupiter:junit-jupiter:5.8.2' | |||
runtimeOnly 'mysql:mysql-connector-java:8.0.28' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api
vs implementation
vs compileOnly
vs runtimeOnly
는 어떤 차이가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implementation : 런타임 + 컴파일 시점에서 모두 사용
compileOnly: 컴파일 시점에서만 사용
runtimeOnly: 런타임 시점에서만 사용
api와 implementation은 의존 라이브러리 수정 시에 차이를 보이는데,
api는 의존하고 있는 모든 모듈들을 rebuild하고,
implementation은 의존하고 있는 본 모듈까지만 rebuild합니다.
따라서 api는 build속도가 implementation에 비해 많이 소요됩니다.
.filter(piece -> piece.getType() == PieceType.KING) | ||
.noneMatch(piece -> piece.getTeam() == team); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChessBoard에서 getter하지 않고, 각 객체에 책임을 위임하는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이미 만들어둔 메서드를 사용하지 않고 있어서 수정했습니다!
.filter(numberOfPawn -> numberOfPawn >= 2) | ||
.mapToDouble(pawnScore -> pawnScore * 0.5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
매직넘버가 있네요 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의미있는 명칭으로 상수화하였습니다!
src/main/java/chess/piece/Piece.java
Outdated
final List<Rank> slicedRanks = IntStream.range(1, ranks.size() - 1) | ||
.mapToObj(ranks::get) | ||
.collect(Collectors.toList()); | ||
if (from.getRank().getIndex() > to.getRank().getIndex()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 분리로 추상화 수준을 높여볼까요?
} | ||
} | ||
|
||
protected void validateVertical(final Position from, final Position to, final Map<Position, Piece> board) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다.
if (!(isDiagonal(from, to) && toPiece.isBlack()) && | ||
!(isFirstPosition(from) && isFirstMoveCondition(from, to)) && | ||
!(isGeneral(from, to) && toPiece.isEmpty())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 분리로 추상화 수준을 높여볼까요?
board = new HashMap<>(); | ||
for (final File file : File.values()) { | ||
for (final Rank rank : Rank.values()) { | ||
board.put(new Position(file, rank), new EmptyPiece()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestFixture로 만드는 건 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반복해서 사용되는 부분인만큼 TestFixture로 만들어보겠습니다!
.isInstanceOf(IllegalArgumentException.class) | ||
.hasMessage("Rook이 이동할 수 없는 경로입니다."); | ||
.hasMessage("같은 Rank가 아니면 움직일 수 없습니다."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 코드 유지보수 👍
DAO를 이름 그대로 바라보면 기준을 정하기 조금 더 수월한 것 같아요! DAO는 Data Access Object, 즉 데이터 접근을 위한 객체죠. 지금은 생소한 개념이어서 많이 헷갈릴 수 있어요! |
우선 요런 고민을 하는 것부터가 OOP적인 사고를 이미 잘하고 계신다는 증거 같아요 👍 ChessBoard가 적은 책임을 가지고 있어서 그리고 지금은 학습하는 단계이기 때문에 객체를 이렇게 많이 생성해도 괜찮을까? 싶을 정도로 분리해봐도 좋다고 생각합니다 ㅎㅎ |
안녕하세요 다니 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 저문~~! 다니입니다 🙌
피드백 반영 감사합니다 👍
기존에도 깔끔했지만, 코드가 더 깔끔해졌네요!
처음부터 구현을 잘해주셔서 큰 변경점이 없었던 것 같아요 ㅎㅎ
코드리뷰 규칙에 따라 이번 미션은 여기서 머지하도록 할게요!
길었던 체스 미션 수고 많으셨어요. 레벨 1도 수고하셨습니다 ✨
방학 푹 쉬시고, 남은 우테코 기간도 응원할게요 🧚♀️
File file = positionPieceEntry.getKey().getFile(); | ||
Rank rank = positionPieceEntry.getKey().getRank(); | ||
PieceType type = positionPieceEntry.getValue().getType(); | ||
Team team = positionPieceEntry.getValue().getTeam(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용하지 않는 지역변수네요 👀
안녕하세요 다니 :) 저문입니다!
그간 잘 지내셨나요?
이번 미션은 구현을 급하게 하고 시간조절을 못해서 그런지 코드가 더 엉망이 된 것 같네요...ㅠㅠ
README도 제대로 정리하지 못하고 제출한게 많이 아쉽네요.
jdbc도 사용하는데 익숙하지 않아서 많이 애 먹었던 것 같아요...
이번 3, 4단계를 진행하면서 궁금했던 점에 대해 남겨보겠습니다!
DAO는 어디서, 어떻게 사용하는 것이 명확한가?
DAO라는 개념을 사용하면서 이것이 어디까지 영향을 미쳐야하는지가 궁금했습니다.
사용을 할 때의 범위는 어떤 기준으로 어떻게 정해야할지 모르겠어서 피드백 강의 내용을 보면서 따라했습니다.
그러다보니 저만의 기준이 사라진 것 같다고 느꼈습니다.
어떤 부분을 어떻게 공부해야할지도 감이 잘 안왔습니다😭
다니의 조언이 필요합니다!
점수를 계산하는 역할을 ChessBoard가 가져도 되는가?
제가 가장 부족한 부분은 객체지향적 사고라고 생각을 합니다.
따라서 이번에도 점수계산을 하는 역할을 어디에 둬야할지 고민을 많이했고,
제가 내린 결론은
ChessBoard에서 하는 것은 크게 문제가 되지 않는다.
였습니다.board가 가진 역할이 그렇게 많지 않다고 생각했기 떄문입니다.
이에 대한 다니의 생각이 궁금합니다.
리뷰에 미리 감사드립니다🙇🏻♂️🙇🏻♂️🙇🏻♂️