-
Notifications
You must be signed in to change notification settings - Fork 415
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
[1,2단계 - 체스] 레디(최동근) 미션 제출합니다. #678
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.
안녕하세요, 레디 😄 리뷰어 제이미입니다.
코멘트 조금 더 남겨두었어요!
이번 Request Changes 반영을 하고 나면, 1~2단계 요구사항은 충분히 충족하는 것 같아서 3~4단계로 넘어가봐도 좋겠네요.
즐거운 주말 보내세요! 💪
@@ -0,0 +1,61 @@ | |||
# 체스 미션 |
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.
우왓 감사합니다 미션을 바라보는 새로운 눈이 생긴 거 같네요🙇♂️
주신 질문에 나름의 답을 해보자면,,
- 코드를 작성하기 전에 설계를 한번 하고 들어갔다면, 초반에 다른 길로 새지 않았을 거 같습니다. (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.
매 미션마다, 미션에서의 아쉬웠던 점과 좋았던 점을 고민해본다면
레디의 강점과 약점을 분석하고, 어떠한 방향으로 나아가는 것이 좋을지 지표가 될 수도 있습니다 😃
@@ -0,0 +1,34 @@ | |||
# 체스 미션 |
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.
👍 덧붙이자면, PR을 올린다는 것은 다른 개발자들이 해당 내용들을 확인할 수 있다는 의미겠죠? 😃
물론 마감 시간이 촉박했으리라 짐작하지만, PR을 올린 후 코멘트를 변경하거나, 추가적으로 커밋 푸시가 생기거나 했을 때는
해당 PR을 확인하는 다른 개발자의 리뷰를 방해할 수 있다는 점으로 접근해보면 좋을 것 같아요 😉
final InputView inputView = new InputView(new Scanner(System.in)); | ||
final OutputView outputView = new OutputView(); | ||
final InputController inputController = new InputController(inputView, outputView); | ||
|
||
final ChessController chessController = new ChessController(inputController, outputView); | ||
chessController.run(); |
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.
예외시 재입력 및 입력 값을 Model로 변환하는 작업을 InputController 에서 처리해주었었는데, 현재 미션에선 이 InputController 가 재구실을 하지 못한 것 같습니다
😄
의존성 주입을 위하여 InputView와 OutputView를 Application에서 생성한 후 넣었습니다!
의존성 주입을 해주는 이유는 무엇인가요?
적절한 이름일까 의문이 생기는군요ㅎㅎ 조금 더 고민해보겠습니다ㅏ!
👍
|
||
public ChessController(final InputController inputController, final OutputView outputView) { | ||
this.inputController = inputController; | ||
this.outputView = outputView; | ||
} | ||
|
||
public void run() { |
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.
최종적인 목표는 예외가 발생하면 StackTrace를 보여주는 것이 아니라 예외 메시지를 출력하고 재입력 받는 기능입니다!
위 내용은 이해했습니다.
하지만 시간이 모자라서 구현하지 못했다면, 그 때는 재입력을 받지 않더라도 예외 메시지만 출력하는 부분까지는 구현을 했어도 좋았을 것 같아요!
이 프로그램에서는 괜찮지만 stacktrace가 노출되면 보안적으로 취약한 부분이 되기도 하기 때문에 잘 챙겨주시면 좋습니다 💪
GameBoard gameBoard = new GameBoard(); | ||
gameBoard.setting(); |
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.
아무런 기물이 없는 체스 게임 보드를 만들고 싶어서 작업을 나눴습니다! 아무런 기물이 없는 체스 게임 보드 만들고 그 위에 원하는 기물들 몇개만 올리는 형태가 된다면 테스트가 쉬워질 거 같았습니다.
의도는 좋습니다! 아래 부분을 고민해보면 좋겠네요!
- 기물이 없는 체스보드의 경우, 체스보드의 역할을 제대로 해줄 수 있는지
- 원하는 기물들을 받아 체스보드를 만들어주는 생성자(또는 정적팩터리메서드)로도 동일한 효과를 얻을 수 있는 게 아닐지
setupStartingPosition()
이름의 정적팩터리메서드를 생성해주었네요 😃
정적 팩터리 메서드의 경우도 주로 사용되는 명명규칙이 있는데 이를 한 번 알아보는 것도 좋을 것 같네요!
src/main/java/model/Command.java
Outdated
public static final int HEAD_INDEX = 0; | ||
public static final int CURRENT_INDEX = 1; | ||
public static final int NEXT_INDEX = 2; | ||
|
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/model/Command.java
Outdated
public boolean isAvailableSize(int targetSize) { | ||
return size == targetSize; | ||
} |
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.
실상 같은 size인지 비교하는 메서드네요! isAvailableSize
라는 의미가 적절한가요?
그리고 해당 부분은 테스트되지 않았네요! 테스트 없이 구현된 메서드의 기능들을 체크해볼까요?
모든 기능을 TDD로 구현해 단위 테스트가 존재해야 한다. 단, UI(System.out, System.in) 로직은 제외
import java.util.List; | ||
import model.Command; | ||
|
||
public class Initialization { |
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.
이 친구는 어떻게 보면 GameStatus가 아니라 GameStatus를 위한 Factory같은 느낌이군요!
조금 더 적절한 클래스명과 메서드명을 지어줘도 좋을 것 같아요!
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.
Initalization 클래스명을 StatusFactory로 변경하고 메서드 명을 create로 변경해보았습니다!
사실 상태 패턴이 익숙치가 않아 어떻게 지어야 할지 감이 안잡힙니다...ㅎ
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.
요 친구는 상태패턴과는 관계가 없는 Factory 인 것 같아요! (상태패턴을 만들어주고는 있지만요!)
디자인패턴을 전부 다 알 필요는 없지만, 사용하는 or 사용한 디자인패턴에 대해서는 조금 더 알아보고 가는 것도 좋겠네요 😉
버퍼 기간을 활용해봐도 좋구요!
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.
아하 버퍼 기간 야무지게 활용해보겠습니다!!!
import java.util.function.Function; | ||
import java.util.stream.Collectors; | ||
|
||
public enum ErrorCodeMessage { |
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.
View를 위한 ErrorCodeMessage를 만들었군요!
Domain에 View에 대한 내용을 포함하지 않기 위해 이러한 작업을 했다고 예상되는데요.
이렇게 분리했을 때의 실질적인 장/단점은 무엇이 있었을까요?
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.
domain에서 view를 걷어냄으로써 자주 변경될 수 있는 view에 수정이 생겼을 때 view 패키지에서만 수정을 하고 domain은 건들이지 않아도 된다는 장점이 있습니다!
단점으로는 커스텀예외의 단점과 비슷하게 관리해야하는 코드가 증가라고 생각합니다
} | ||
|
||
@DisplayName("해당 위치에 기물이 없는 경우 예외가 발생한다.") | ||
@Test |
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.
고민할만한 포인트네요!
new Moving(E4, E5);
를 move 했을 때 PieceDoesNotExistException
예외가 발생하는 부분을 검증하는 것이
기물이 없는 경우 예외가 발생함을 모두 보장해줄 수 있을까요 ?
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.
테스트가 빈약하다/빈약하지 않다보다
구현된 테스트가 레디의 의도대로 구현되었는가?가 궁금했어요 😃
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.
꼼꼼한 리뷰 감사드립니다 제이미!😀
주신 코멘트 이외에 pawn을 추상클래스로 변경하고 blackPawn과 whitePawn 만들이 pawn을 상속받는 형태로 구현하였습니다
덕분에 중복되는 코드를 제거할 수 있었습니다!
@Override | ||
public String toString() { | ||
return PieceType.from(this).getValue(); | ||
} |
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.
아 3단계 요구 사항으로 게임 종료가 있었군요! 그럼 그때로 미뤄두도록 하죠 ㅎㅎㅎ
2번 같은 경우는 게임에서 승리
가 플레이어의 목적이라고 생각하여 구현하려 했었는데, 생각해보니 과연 플레이어의 목적이 게임에서 승리
가 맞을까? 라는 고민이 생겼네요. 질 수도 있지!
src/main/java/dto/ChessBoardDto.java
Outdated
private static final String FILE_GUIDE_LINE = "abcdefgh"; | ||
private static final String EMPTY_POINT = "."; | ||
private static final String RANK_GUIDE_LINE = " %s"; |
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.
으엇 예리하십니다!!! 👍👍
놓친 부분이네요
|
||
import constant.ErrorCode; | ||
|
||
public class CustomException extends RuntimeException { |
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.
우선 제가 커스텀 예외를 사용한 이유는,,,
이전에는 IllegalArgumentException 안에 각 예외 상황에 따라 다른 예외 메시지
를 넣어 구분을 지어주었습니다.
그러다 공통된 예외가 생기다 보니 중복되는 예외 메시지들이 보였고, 이들을 한 곳에서 관리해줄 녀석이 필요했습니다.
그렇게 한 곳에서 어떻게 관리할 지 고민하다가 내가 표현하고 싶었던 것은 예외 상황
이고 이를 메세지로서 표현했었는데, 메세지 보다는 커스텀 예외가 더 명확하게 전달할 수 있다고 생각하여 사용하였습니다!
장점
- 예외 상황을 명확하게 전달할 수 있다.
단점
- 관리해야하는 코드가 들어난다.
outputView.printChessBoard(ChessBoardDto.from(chessBoard)); | ||
outputView.printCamp(chessBoard.getCamp()); |
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.
해당 방식으로 변경하였습니다! 👍👍
@@ -0,0 +1,15 @@ | |||
package constant; | |||
|
|||
public enum ErrorCode { |
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.
맞네요!! 그럼 ErrorCode를 좀 더 세분화 해보겠습ㄴ디ㅏ💪
} | ||
|
||
@DisplayName("해당 위치에 기물이 없는 경우 예외가 발생한다.") | ||
@Test |
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.
테스트 케이스가 빈약하다는 의견이였을까요?!
케이스를 좀 더 추가하였습니다!
import java.util.function.Function; | ||
import java.util.stream.Collectors; | ||
|
||
public enum ErrorCodeMessage { |
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.
domain에서 view를 걷어냄으로써 자주 변경될 수 있는 view에 수정이 생겼을 때 view 패키지에서만 수정을 하고 domain은 건들이지 않아도 된다는 장점이 있습니다!
단점으로는 커스텀예외의 단점과 비슷하게 관리해야하는 코드가 증가라고 생각합니다
@@ -0,0 +1,61 @@ | |||
# 체스 미션 |
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를 이차원 리스트로 구현했던 점, 현재는 맵)
- 시간이 없을 때 요구사항이 아닌 것들을 포기할 수 있을 거 같습니다. 제일 처음 제출을 하고 이런 고민이 들었습니다. 이번 미션의 요구사항중 클린코드도 있었는데 일단은 돌아가는 코드를 만들겠다고 너무 클린코드 재껴버렸던 것은 아닌지,, 조금은 아쉬운 감이 있었습니다 :)
final InputView inputView = new InputView(new Scanner(System.in)); | ||
final OutputView outputView = new OutputView(); | ||
final InputController inputController = new InputController(inputView, outputView); | ||
|
||
final ChessController chessController = new ChessController(inputController, outputView); | ||
chessController.run(); |
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.
안녕하세요, 레디 😄 리뷰어 제이미입니다.
간단한 코멘트들은 남겨두었으니, 다음 단계 작업하시면서 같이 고민/반영해주세요!
관련된 코멘트들은 모두 현재 PR이 아니라, 새로 작업하는 3, 4단계 PR에 올려주세요 😃
지난번 Request changes 때 말씀드렸듯이, 1/2단계는 모두 충족하여 Approve & Merge합니다.
다음 단계도 화이팅입니다. 💪
final InputView inputView = new InputView(new Scanner(System.in)); | ||
final OutputView outputView = new OutputView(); | ||
final InputController inputController = new InputController(inputView, outputView); | ||
|
||
final ChessController chessController = new ChessController(inputController, outputView); | ||
chessController.run(); |
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.
인터페이스를 사용하지 않는데도 느슨한 결합이 되었다고 할 수 있을까요?
레디의 의도에 따라 구현된 코드인지 한 번 더 체크해볼까요?
@@ -0,0 +1,61 @@ | |||
# 체스 미션 |
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.
매 미션마다, 미션에서의 아쉬웠던 점과 좋았던 점을 고민해본다면
레디의 강점과 약점을 분석하고, 어떠한 방향으로 나아가는 것이 좋을지 지표가 될 수도 있습니다 😃
|
||
import constant.ErrorCode; | ||
|
||
public class CustomException extends RuntimeException { |
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/model/ChessBoard.java
Outdated
private static final Map<File, Function<Camp, Piece>> startingPosition = new EnumMap<>(File.class); | ||
|
||
static { | ||
startingPosition.put(File.A, Rook::new); | ||
startingPosition.put(File.B, Knight::new); | ||
startingPosition.put(File.C, Bishop::new); | ||
startingPosition.put(File.D, Queen::new); | ||
startingPosition.put(File.E, King::new); | ||
startingPosition.put(File.F, Bishop::new); | ||
startingPosition.put(File.G, Knight::new); | ||
startingPosition.put(File.H, Rook::new); | ||
} |
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.
그러면 어떻게 개선해볼 수 있을까요?
import java.util.List; | ||
import model.Command; | ||
|
||
public class Initialization { |
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.
요 친구는 상태패턴과는 관계가 없는 Factory 인 것 같아요! (상태패턴을 만들어주고는 있지만요!)
디자인패턴을 전부 다 알 필요는 없지만, 사용하는 or 사용한 디자인패턴에 대해서는 조금 더 알아보고 가는 것도 좋겠네요 😉
버퍼 기간을 활용해봐도 좋구요!
|
||
public static final int HEAD_INDEX = 0; | ||
|
||
private final Pattern pattern; |
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.
조금 더 이름을 잘 지어줘봐도 좋을 것 같아요.
또한 Pattern.compile
는 중복이 아닐까요?
|
||
public static void validate(List<String> values) { | ||
if (values == null || values.isEmpty()) { | ||
throw new InvalidCommandException(ErrorCode.INVALID_COMMAND); |
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.
모든 기능을 TDD로 구현해 단위 테스트가 존재해야 한다. 단, UI(System.out, System.in) 로직은 제외
핵심 로직을 구현하는 코드와 UI를 담당하는 로직을 구분한다.
UI 로직을 InputView, ResultView와 같은 클래스를 추가해 분리한다.
한번 쭉 점검해줄까요?
해당 메서드에서는 values, 아래 메서드에서는 input을 쓰고 있는데, 이름에 대해서도 일관성을 가져가면 좋을 것 같아요.
public static final int HEAD_INDEX = 0; | ||
public static final int CURRENT_POSITION_INDEX = 0; | ||
public static final int NEXT_POSITION_INDEX = 1; |
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.
public ! ! 상수로 추출할 때 실수한 부분이라면 상수 추출시 C를 한 번 더 눌러서 기본 접근제어자를 바꿔주는 것도 방법이겠죠?
piece들에 있는 숫자는 매직넘버로 빼지 않고, 해당 부분은 매직넘버로 뺀 이유도 궁금하네요 😃
} | ||
|
||
public List<String> getBody() { | ||
return body; |
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.
body를 수정할 수 있겠군요 💪
getter로 보내줄 때 가변인 상태로 보내주는 것이 의도된 동작인지 검토해볼까요?
public static Pawn create(Camp camp) { | ||
if (camp == Camp.BLACK) { | ||
return new BlackPawn(); | ||
} | ||
return new WhitePawn(); | ||
} |
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.
Pawn은 BlackPawn과 WhitePawn의 부모인데, 구현체를 알아도 되는걸까요?
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.
리뷰 감사드립니다! 그럼 3,4단계 때 보아요😁
|
||
private CommandLine readCommandLine() { | ||
try { | ||
List<String> command = inputView.readCommandList(); |
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.
필사적으로 전부 다 붙이려고하는데 요 친구는 놓쳤네요😌
그럼 final 왜 붙이는데?? 라고 누군가 물어봤을 때 답해보자면, 이 코드는 추후에 내가 아닌 다른 사람이 볼텐데, final을 붙임으로써 "이거 이제 멋대로 건들이지마!!"라는 메시지를 전달할 수 있다고 생각하여 필요한 곳에 전부 붙여주고 있습니다
final이 붙어서 좀 가독성이 떨어지는 느낌도 있지만 이제 익숙해져 없으면 허전한거 같기도...ㅎㅎ (근데 요곤 왜 놓쳤지 보니까 저어기 인터페이스에서도 안붙어 있군요)
private static final String FILE_GUIDE_LINE = "abcdefgh"; | ||
private static final String RANK_GUIDE_LINE_FORM = " %s"; | ||
private static final String EMPTY_POINT = "."; | ||
private static final int BOARD_SIZE = 8; |
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.
웁스 아닙니다!!
return EMPTY_POINT + paddedRankGuidLine(file, rank); | ||
} | ||
|
||
private static String paddedRankGuidLine(final File file, final Rank 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.
이름짓기가 진짜 어려운거 같아요...ㅎㅎ
todo 붙여두고 지금은 시간없으니 나중에 이쁜 이름 지어줘야지 했다가 다시 지으러 왔을 땐 이게 대충 괜찮네 하고 넘어가기 일쑤..
메서드가 하는 일에 맞춰 의도를 전달하는 이름 짓는 연습이 더 필요해보이네요😗
public static Pawn create(Camp camp) { | ||
if (camp == Camp.BLACK) { | ||
return new BlackPawn(); | ||
} | ||
return new WhitePawn(); | ||
} |
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/model/ChessBoard.java
Outdated
private static final Map<File, Function<Camp, Piece>> startingPosition = new EnumMap<>(File.class); | ||
|
||
static { | ||
startingPosition.put(File.A, Rook::new); | ||
startingPosition.put(File.B, Knight::new); | ||
startingPosition.put(File.C, Bishop::new); | ||
startingPosition.put(File.D, Queen::new); | ||
startingPosition.put(File.E, King::new); | ||
startingPosition.put(File.F, Bishop::new); | ||
startingPosition.put(File.G, Knight::new); | ||
startingPosition.put(File.H, Rook::new); | ||
} |
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.
지금 딱 생각나는건 두가지가 있는데 둘다 마음엔 안드네요😅
- Map을 없앤다.
- static final 을 없앤다. (써놓고 보니 너무 이상하네요)
src/main/java/model/ChessBoard.java
Outdated
private static final Map<File, Function<Camp, Piece>> startingPosition = new EnumMap<>(File.class); | ||
|
||
static { | ||
startingPosition.put(File.A, Rook::new); | ||
startingPosition.put(File.B, Knight::new); | ||
startingPosition.put(File.C, Bishop::new); | ||
startingPosition.put(File.D, Queen::new); | ||
startingPosition.put(File.E, King::new); | ||
startingPosition.put(File.F, Bishop::new); | ||
startingPosition.put(File.G, Knight::new); | ||
startingPosition.put(File.H, Rook::new); | ||
} |
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.
3,4 단계에서 계속 고민해보겠습니다!ㅎㅎ
final InputView inputView = new InputView(new Scanner(System.in)); | ||
final OutputView outputView = new OutputView(); | ||
final InputController inputController = new InputController(inputView, outputView); | ||
|
||
final ChessController chessController = new ChessController(inputController, outputView); | ||
chessController.run(); |
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.
오오오 좋습니다! 질문을 너무 맛있게 해주셔서 코딩할 맛이 나는데요 😁
import java.util.List; | ||
import model.Command; | ||
|
||
public class Initialization { |
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.
아하 버퍼 기간 야무지게 활용해보겠습니다!!!
안녕하세요 제이미ㅣ!
레디라고 합니다 :)
리뷰어로 만나서 만갑습니다 😀
기간이 꽤나 짧았기에 클린코드에 비중을 두었다기 보다는 일단 돌아가도록 만들었습니다
코드를 작성하면서도 일단 돌아가는 게 중요해!라며 흐린 눈 하며 코드를 작성하였습니다...ㅎㅎ
구조
처음에는 GameBoard를 이차원 리스트로 구현을 하였고 그 안에 Piece 를 넣는 식의 구조였습니다.
보시다시피 객체 그래프(?)가 너무 깊어 지는 느낌이여서 GameBoard 가 이차원 리스트로 Piece 정보를 가지고 있는 형태가 아니라 Map 에 키 값으로 위치를 저장하고 그 위치에 Piece를 저장하는 식으로 변경을 하였습니다.
변경 커밋
이렇게 변경하였더니 말들을 움직일 때도 요소의 값들을 스왑할 필요도 없이 그냥 맵의 키를 삭제하고 추가하는 방식으로 구현할 수 있어 좋았고, 또 아무런 Piece가 없는 빈 공간에 대해서도 크게 신경쓸 필요가 없어 좋았습니다.
이전 미션에선 리뷰어한테 질문할 거리가 하나씩은 있었던 거 같은데 체스 미션은 구현이 바빠서 그랬는지 질문이 없는 거 같네요 😅
리팩터링 하다가 생기겠죠...? ㅎㅎㅎ
리뷰 미리 감사드립니다!! 😊