[🚀 사이클1 - 미션 (보드 초기화 + 기물 이동)] 이안 미션 제출합니다.#263
[🚀 사이클1 - 미션 (보드 초기화 + 기물 이동)] 이안 미션 제출합니다.#263Eian1106 wants to merge 40 commits intowoowacourse:eian1106from
Conversation
sihyung92
left a comment
sihyung92
left a comment
There was a problem hiding this comment.
안녕하세요 이안, 리뷰어 웨지입니다.
질문 하신 것들은 본문 코멘트 속에 남겼으니 확인해보셔요.
질문이나 고민은 편히 남겨주셔요. 화이팅입니다
| import domain.PieceType; | ||
| import domain.Position; | ||
|
|
||
| public class Piece { |
There was a problem hiding this comment.
Piece 자체는 'new Piece()'로 생성되면 아무 역할을 못하는 객체입니다. 구체적인 타입핑이 있어야 하는 객체인데요.
또 canMove도 모든 기물이 오버라이드하는 구조인데, Piece 자체의 canMove는 무조건 true를 반환하네요. 오버라이드를 놓치면 오류가 발생하는 구조여서 실수에 취약한 구조입니다.
Piece를 abstract class로 선언하고, canMove를 추상 메서드로 두면 하위 클래스에서 반드시 구현해야 하는 제약이 생기겠죠.
There was a problem hiding this comment.
맞습니다!.. 남겨주신 리뷰를 보고 해당 코드를 작성할 때 추상클래스를 왜 떠올리지 못했을까? 를 돌아보면 추상클래스에 대해 제대로 알지 못했던 것 같습니다.
추상 클래스에 대해 다시 한 번 학습을 진행하였습니다.
- 추상 클래스의 경우 인스턴스를 생성할 수 없다.
- 추상 클래스의 추상 메서드는 상속 받는 자식 클래스가 반드시 오버라이딩 해서 사용해야 한다.
- 제약을 걸 수 있다.
현재 장기는 Piece를 생성하여 사용하지 않습니다.
canMove는 모든 기물이 구현해야하는 내용입니다.
위의 내용을 보아 Piece를 추상 클래스로 변경하는 것이 맞습니다! 감사합니다 😄
|
|
||
| public class Piece { | ||
|
|
||
| private final PieceType pieceType = PieceType.NONE; |
There was a problem hiding this comment.
부모 클래스의 pieceType이 PieceType.NONE으로 고정되어 있고, 각 하위 클래스에서도 pieceType 필드를 다시 선언하고 getPieceType()을 오버라이드하고 있는데요,
생성자를 통해 PieceType을 받도록 해보셔요.
| boolean jumping = false; | ||
|
|
||
| try { | ||
| while (!jumping) { |
There was a problem hiding this comment.
while문은 치명적인 장애를 유발할 수 있는 문법이어서 사용 시 주의를 기울여야 합니다.
저는 극단적으로 무조건 탈출식을 넣습니다. int loop를 넣어서, loop가 10,000 (비즈니스 로직 상 버그가 아니면 도달할 수 없는 수) 를 넣는 식으로요.
게다가 checkCannonJumping이라는 메서드는 인터페이스에 의존하고 있어 구현체에 따라 break에 도달하지 못 할 수도 있어 위험한 선택입니다. 인터페이스의 의미를 생각해보세요.
향후 수정하기 어려울 코드에 미리 추상화를 씌워 대비하는 겁니다. 그렇다면 코드에서도 인터페이스의 계약에 의해서만 행동할 필요가 있는데, ExistBoard의 새로운 구현체를 만들 사람이 이 안에서의 동작을 숙지하고 있지 못하다면 프로그램이 즉시 정지합니다.
다른 방식으로 해결해보세요.
There was a problem hiding this comment.
while문은 치명적인 장애를 유발할 수 있는 문법이어서 사용 시 주의를 기울여야 합니다.
저는 극단적으로 무조건 탈출식을 넣습니다. int loop를 넣어서, loop가 10,000 (비즈니스 로직 상 버그가 아니면 도달할 수 없는 수) 를 넣는 식으로요.
이런식으로 장애를 대응할 수 있다는 것은 처음 알았습니다!.. 무궁무진 하네요..!
인터페이스의 의미를 생각해보세요.
인터페이스를 다시 한번 생각해보았습니다.
-
인터페이스는 반환값, 매개변수만 결정하고, 구현은 구현체에서 하도록 됩니다.
-
이말은 즉, 반환값(예: boolean)이라면 true던, false던 통제 부분이 아니게 됩니다.
결국 제 코드의 경우. while문에서 인터페이스의 결과에 의존적인데, 만약 구현체로 while문이 빠져나올 수 없는 값만 반환하게 된다면 장애가 난다. 런타임에 어떤 구현체가 들어올지 모른다. 라는 의미로 이해했습니다!
src/main/java/domain/Position.java
Outdated
|
|
||
| public class Position { | ||
|
|
||
| public static int MAX_X_VALUE = 8; |
There was a problem hiding this comment.
final이 빠져있어 외부에서 값을 바꿔버릴 수 도 있습니다. 보호해줍시다.
| return this.camp == camp; | ||
| } | ||
|
|
||
| public boolean isDifferentPieceType(Piece piece) { |
There was a problem hiding this comment.
isDifferentPieceType 메서드가 this.getClass() != piece.getClass()로 비교하고 있네요. PieceType enum이 있는데 이걸 활용하지 않고 클래스 비교를 하는 이유가 궁금해요.
this.getPieceType() != piece.getPieceType()으로 비교하면 PieceType enum을 도입한 의미가 살아나고, 클래스 구조 변경에도 유연해집니다.
There was a problem hiding this comment.
PieceType enum이 있는데 이걸 활용하지 않고 클래스 비교를 하는 이유가 궁금해요.
1.1 단계인 보드 초기화
1.2 단계인 기물의 이동
위의 내용을 구현 완료할 때 까지도 enum도입을 하지 않았기에, getClass()를 통해 비교하고 있었습니다.
모든 기능 구현 후 페어와 View에서 각 기물을 어떻게 구분하고 출력하지? 고민하던 중
.class는 구체적인 클래스타입에 의존하게 되니, PieceType을 추가해야겠다가 되었습니다.
따라서 추후에 추가된 enum을 제대로 활용하지 못했습니다.
PR을 아직 올리지 않은 팀이 약 5팀 이였는데, 그 중 한 팀 이었기에 조급함도 있었던 것 같습니다 😓
해당 내용은 바로 수정했습니다!
src/main/java/domain/Board.java
Outdated
| private final Map<Position, Piece> board = new HashMap<>(); | ||
|
|
||
| public void generatePiecesBy(Camp camp, ElephantFormation elephantFormation) { | ||
| PieceGenerator pieceGenerator = new PieceGenerator(); |
There was a problem hiding this comment.
상태가 없는 helper 클래스인데, 인스턴스를 선언하지 말고 static 메서드로 처리해보세요.
| void X_예외_값_입력_오류_검증(int x) { | ||
| assertThatThrownBy(() -> new Position(x, 4)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessageContaining("[ERROR]", "x 좌표는"); |
There was a problem hiding this comment.
사소하지만 x, y 좌표 오류일 때 메시지 띄어쓰기가 차이가 있네요.
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| public enum ElephantFormation { |
There was a problem hiding this comment.
상 차림의 경우 전략패턴 사용과 enum 사용 중 리뷰어님은 어떤 선택을 하실지 궁금합니다!
우선 코드에 정답은 없다는 걸 전제로 하고요. 제 선택을 물어보셨으니 재미로만 보셔요.
저라면 마상마상 마상상마 상마상마 상마마상 을 나타내는 열거형(enum) 하나 만들고, 필드는 선언하지 않습니다.
고민되는 지점은 String 필드로 description을 둘까 말까인데, 아마 둔다면 "마상마상", "마상상마" 이렇게 넣어 둘거같네요. (비즈니스 사용 용도나 뷰 출력용이 아니라 로깅이나 파악하기 위한 목적으로)
그리고 거기에 대한 포지션 정렬은 상차림 하는 객체 (여기선 PieceGenerator)에서 enum을 보고 처리합니다.
포지션 규칙을 이 enum에 넣지 않는 이유는 position 계산에 대한 책임이 흩어지기 때문이에요. position을 고칠 때 이 코드 저 코드 봐야하는게 싫어서 세팅 알고리즘은 한 객체에 몰아넣습니다.
전략패턴은 장기의 도메인 특성상 변경될 여지가 작으므로 고려하지 않습니다.
| @@ -0,0 +1,88 @@ | |||
| package domain.pieces; | |||
There was a problem hiding this comment.
각 기물들의 이동 위치 계산 메서드에(canMove) 동일한 로직의 중복이 많이 있습니다. 움직임 방향만 다르게 주입하여 동일한 로직을 재사용 하고싶은데, 어떤 것을 공부하고 적용하면 좋을까요? 갈피를 못 찾고 있습니다.
canMove 내에서 반복되는 패턴이 뭔지 식별해보세요.
- 포지션 변경
- 보드 내 동일한 기물 있는지 확인
우선 포지션 변경에 대해, from position에서 가능한 경우를 컬렉션으로 만들고 계신데요.
이걸 Piece가 직접 하지 말고 따로 분리하셔서,
position이 갈 수 있는 루트를 만드는 로직을 클래스로 따로 짜보세요. 장기에서 가능한 모든 move에 대해서요.
General과 Guard는 같은 로직을 쓰게 될거고요. Elephant와 Horse는 거의 똑같은데 한끝만 다른 클래스를 갖게 될거고요. (boolean 필드 하나만 추가해주고, true면 elephant, false면 horse..)
이게 완료되면 2번도 해보시면 되고요. 2번은 기물마다 route에 있는지 없는지에 대해 뛰어넘고 말고를 결정해야되는데, 경우의 수가 2가지 뿐이잖아요. 뛰어넘는다 / 뛰어넘지 않는다
그럼 boolean 하나로 또 분기를 나눌 수 있는데, 이 코드는 템플릿 메서드 패턴으로 Piece에서 위에 구현한 move를 감싸서 해주면 되겠죠. 따로 객체화해서 분리할 정도는 아니고 부모 객체가 해주면 적당한 수준이니 Piece에 두고요.
어떤 방식으로 작업하라는 내용의 가이드는 지양하는 편인데 무어라도 힌트가 필요하신거 같아 길게 남겨봤습니다.
저는 우테코 때 남에 꺼 많이 베꼈어요. 다른 크루들은 어떻게 구현했는지 보고, 단순히 복붙해오는게 아니라, 어떤 의도로 저렇게 짠건지 좋은 패턴이 있다면 배우고 흡수하셔요. 0에서 하는 것보다 모방에서 시작하는 게 더 좋은 학습법 이어서요
| } | ||
|
|
||
| @ParameterizedTest | ||
| @ValueSource(ints = {-5, -1, 10, 12, 1000}) |
There was a problem hiding this comment.
ValueSource를 설정할 때 경계값들에 대해 잘 세팅해주고 계시네요.
각 기물들은 이동 규칙에 따라 Direction을 가지고있다. route에서 이동 position을 계산한다.
|
안녕하세요 웨지! 늦은 이유는 아래와 같습니다. 😓
큰 부분에서, 개선해본 점은 다음과 같습니다.
#263 (comment) 사실 해당 내용에 대해 이해하는 데 시간이 조금 걸렸습니다. 사이클1 첫 리뷰요청때는 머릿속 청사진 조차 그려지지 않았다면, 이제는 조금 그려진 듯한 느낌입니다 |
3 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?어떤 부분에 집중하여 리뷰해야 할까요?
페어와 궁금증이 비슷하여, 해당 질문들을 취합했습니다!
질문할 점
try-catch
현재 코드 여러 곳에서 try-catch가 사용되고 있습니다. 어쩌면 남발했다고 볼 수 있는데, 에러 처리의 관리 부족인지, 적절한 사용인지 궁금합니다.
또한
Elephant와Chariot내부 catch문에서 특정한 행동을 하지 않아도 되기에 비어있는 곳도 있습니다.아무 행동을 하지 않는다면 의도적으로 catch를 비워도 될까요?
현재 모든 기물에서 이동 가능한 위치를 확인할 때, Board의 크기를 기물에 전달하지 않기 위해서 계속해서 position(생성 시 좌표가 보드 내에 있는 지 확인함)을 생성하고 있습니다. 더 이상 position이 정상 생성되지 않을때까지 반복하기 때문에 try-catch를 사용하고 있습니다. Board크기를 전달하지 않기 위해 불필요한 예외처리 로직을 추가해서 클린하지 않은 코드를 작성한 것 같습니다
position의 MAX 값을 가져와 사용하여 if문을 통해 최대 값 이후 이동하지 않기 보다, try-catch문을 사용중입니다. 괜찮은 방식인지, 더 좋은 방식이 있을지 궁금합니다!
View의 역할
Test도 메서드 분리를 해야하는가?
중복 메서드에 대해
BoardGenerator의 말 생성 메서드를 하나로 재사용 하기 위해서 Factory 메서드를 적용해볼까? 했지만, 적용하기까지 좋은 아이디어가 떠오르지 않았습니다.
또한 각 기물들의 이동 위치 계산 메서드에(canMove) 동일한 로직의 중복이 많이 있습니다. 움직임 방향만 다르게 주입하여 동일한 로직을 재사용 하고싶은데, 어떤 것을 공부하고 적용하면 좋을까요? 갈피를 못 찾고 있습니다.
전략패턴과 enum
페어: @yejinkimis