[🚀 사이클1 - 미션 (보드 초기화 + 기물 이동)] 코코 미션 제출합니다.#262
[🚀 사이클1 - 미션 (보드 초기화 + 기물 이동)] 코코 미션 제출합니다.#262yejinkimis wants to merge 27 commits intowoowacourse:yejinkimisfrom
Conversation
echo724
left a comment
echo724
left a comment
There was a problem hiding this comment.
안녕하세요 코코!
전체적으로는 콘솔 UI까지 신경 쓰신 부분이나, OutputView 쪽에 표시 책임을 모아두신 부분처럼 단순히 기능만 맞춘게 아니라 프로덕트 완성도까지 고민하신 흔적이 보여서 좋았습니다 ㅎㅎ
질문 주신 부분 답을 해보자면:
try-catch
사실 이 부분은 취향?의 영역이라고 생각도 드는게 저는 Try-catch를 별로 좋아하진 않아서 Result 타입에 Error를 추가하는 형식으로 구현하는걸 좋아하는데요, 이런 경우 반대로 Error 타입 처리를 위한 코드가 늘어나다 보니 가독성이 떨어지는 경우도 있습니다. 코코의 경우 아무래도 try-catch를 했지만 suppress? (무시)하는 경우가 종종 있어서 고민이신 것 같은데, 아무래도 이게 예외는 아닌 상황이라 애매하기도 하죠. 저같은 경우에는 Optional<> 타입을 고민해볼 것 같습니다. 예외는 아니지만 그럼에도 기물에 의해 못움직일 수 있는 경우를 나타내기 좋을 것 같아요. 비지니스 관련 제약이 정말 예외인가?에 대한 고민도 해보시면 좋을 것 같구요. 추가로 try-catch가 If에 비해서 훨씬 비쌉니다. 이 부분 찾아서 확인해보시면 좋을듯 하네요!
View의 역할
저는 사실 괜찮다는 생각하는데 그 이유는 1. 입력과 검증이 가까워서 비용도 저렴하고 재시도 하기 쉬움 등의 다양한 장점이 있음 2. 범용적인 input의 경우 어려울 수 있지만 대부분 한 값 객체를 위한 입력인 경우가 대부분 이 경우에는 VO를 반환하는게 좋은 것 같습니다. 어차피 string 반환해서 외부에서 바로 생성할 것이라면 Input에 가깝게 둬야하지 않나 생각합니다. 코코는 어떻게 생각하시나요?
Test 분리
기본적으로 테스트는 어쩔 수 없다고 생각합니다. 한 테스트에서 담고자 하는 플로우가 길 수 도 있고, 나중에 Mock같은 경우도 설정하다보면 코드가 길어질 수 있거든요. 다만 주의해야할 점은, 예를 들어 실제 메서드 A를 테스트 하고자 할때, A를 위해 추가로 생성해줘야하는 의존성이나 코드가 너무 길다면 (BDD에서는 Given 영역) 설계를 제대로 했는지 의심해볼 필요는 있습니다. 그 부분이 길다는 얘기는 한 메서드 실행을 위해 너무 많은 준비물이 필요한 것과 마찬가지거든요.
중복 메서드에 대해
이 부분 커멘트로 남겨둔 것 같습니다. 아무래도 중복을 줄이는데에는 공통적인 부분을 실제로 합쳐보고 아닌것 같다 싶으면 다시 돌려놓는 연습을 좀 해보면 느는것 같은데요, 실제로 돌아가는 코드를 구현해보시고(지금 미션 제출하신 것처럼요) 이미 한 번 짜봤기 때문에 다음에 새로 중복 되는 코드를 생각해서 다시 재작성해보는 것도 도움이 됩니다. 모든 코드는 아니고 그래도 중복이 좀 많이 보이거나 어색한 부분들에 있어서요. 그러면 전에 처음 짤때보다단 빠르고 효율적으로 짜볼 수 있으실 것입니다.
전략패턴과 enum
사실 이 부분 질문이 잘 이해가 안가는데요, (제 문제일 것 같습니다) 전략 패턴과 enum 둘 중에 하나만 선택하는게 어색해서 그런 것 같아요? 그래서 일단 커멘트 기반으로 관련 부분 답 드리는데 이 부분 질문 구체화 해주시면 감사할 것 같습니다. 일단 상 차림에 따른 enum 타입에 따라 generator에서 기물들을 배치하는 것으로 기억하고 있는데요, 이 경우, 전략 패턴이 아닌 것에서 오는 단점이라기 보다는 기물 배치 방식에 있어서 자료 구조가 좀 복잡한게 아닌가 생각이 듭니다. 이 부분 좀 고민 해보시면 좋을것 같은데요, 현재 구조로는 배치 로직을 확인하는게 쉽지 않습니다. 쉽에 어떻게 바꿔라 할 순 없지만, 문제 해결 보다는 Map, Enum을 맞추기 위해 좀 코드가 변형이 된게 아닌가 생각이 드네요? 이 부분 혹시 어떻게 생각하시나요? 그 좀 더 생각을 듣고 싶은데 커멘트로 기물 배치 설계하셨을때 고민하셨던 부분 나눠주시면 좋을 것 같아요. 현재로서는 그 부분이 가장 복잡해보이지 않나 생각이 듭니다.
반영 후 다시 리뷰 요청 주시면 이어서 보겠습니다! 수고 많으셨습니다~
| System.out.println(camp.getCampName() + " 상 차림을 결정해주세요."); | ||
| System.out.println("1. [마 상 마 상]"); | ||
| System.out.println("2. [마 상 상 마]"); | ||
| System.out.println("3. [상 마 상 마]"); | ||
| System.out.println("4. [상 마 마 상]"); | ||
| return Integer.parseInt(sc.nextLine()); |
There was a problem hiding this comment.
혹시 이 부분 상 차림이 추가 된다거나 변경 되는 경우에는 어떻게 될까요?
| private static final String ANSI_RESET = "\u001B[0m"; | ||
| private static final String ANSI_GREEN = "\u001B[32m"; | ||
| private static final String ANSI_RED = "\u001B[31m"; | ||
|
|
||
| // 전각 문자 통일을 위한 설정 | ||
| private static final String EMPTY_SPACE = "．"; // 전각 마침표 (U+FF0E) | ||
| private static final String[] FULL_WIDTH_NUMBERS = { | ||
| "０", "１", "２", "３", "４", "５", "６", "７", "８", "９" | ||
| }; | ||
|
|
||
| private static final Map<PieceType, String> CHO_SYMBOLS = new EnumMap<>(PieceType.class); | ||
| private static final Map<PieceType, String> HAN_SYMBOLS = new EnumMap<>(PieceType.class); | ||
|
|
||
| static { | ||
| CHO_SYMBOLS.put(PieceType.GENERAL, "楚"); | ||
| CHO_SYMBOLS.put(PieceType.CHARIOT, "車"); | ||
| CHO_SYMBOLS.put(PieceType.CANNON, "包"); | ||
| CHO_SYMBOLS.put(PieceType.HORSE, "馬"); | ||
| CHO_SYMBOLS.put(PieceType.ELEPHANT, "象"); | ||
| CHO_SYMBOLS.put(PieceType.GUARD, "士"); | ||
| CHO_SYMBOLS.put(PieceType.SOLDIER, "卒"); | ||
|
|
||
| HAN_SYMBOLS.put(PieceType.GENERAL, "漢"); | ||
| HAN_SYMBOLS.put(PieceType.CHARIOT, "車"); | ||
| HAN_SYMBOLS.put(PieceType.CANNON, "砲"); | ||
| HAN_SYMBOLS.put(PieceType.HORSE, "馬"); | ||
| HAN_SYMBOLS.put(PieceType.ELEPHANT, "相"); | ||
| HAN_SYMBOLS.put(PieceType.GUARD, "士"); | ||
| HAN_SYMBOLS.put(PieceType.SOLDIER, "兵"); | ||
| } |
There was a problem hiding this comment.
오오 콘솔 UI를 신경 쓴 부분이 보이네요! 좋습니다 ㅎㅎ 미션에 나오진 않은 부분 같은데 저는 이렇게 완성도를 높은 코드/프로덕트가 좋더라구요. 👍
There was a problem hiding this comment.
추가로 OutputView에서 기물 표시 정보를 담고 있는 것도 매우 좋은 방식인 것 같습니다. 만약에 기물 표시를 바꾼다고 해도 이 곳에서만 변경을 하면 되기 떄문이죠.
| private static Map<PieceLocation, Map<Camp, List<Position>>> initInner() { | ||
| Map<PieceLocation, Map<Camp, List<Position>>> position = new HashMap<>(); |
There was a problem hiding this comment.
이 부분 상당히 복잡한 구조로 보이는데요, PieceLocation은 내부에 Map<Camp, List>를 상태로 들고 있는데, ElephantFormation은 또 Map<PieceLocation, Map<Camp, List>>를 상태로 들고 있네요? Camp 정보를 뺀다던가, 아니면 Map<Camp, List>를 별도의 객체로 둔다던가 등의 조치가 필요할 것 같습니다.
|
|
||
| public class General extends Piece { | ||
|
|
||
| private final PieceType pieceType = PieceType.GENERAL; |
There was a problem hiding this comment.
사실상 이 부분도, 내부적으로는 pieceType이 전혀 사용되고 있지 않네요? 외부에서 key 정도로 사용되는 것 같은데 굳이 필요할까 싶기도 합니다. 상속체 타입을 보고 알 수는 없을까요? 아니면 타입을 키로 두는 설계가 괜찮은지도 고민해볼 필요가 있습니다. 제 기준에서는 현재 PieceType, PieceLocation은 둘다 동일한 enum 상태들을 가지고 있기도 하고, 각 상속체는 또 따로 타입이 있기 때문에 만약 피스 타입이 추가되는 요구사항이 생긴다면 변경 범위가 꽤 클 거 같고 실수를 할 수 도 있을 것 같거든요. 예를 들어 한 쪽 이넘에는 추가를 했는데 다른쪽 이넘에는 추가를 안했다던지요. 이렇듯 같은 정보는 최대한 중복이 없도록 하는 것이 좋습니다.
| HAN("한나라"), | ||
| CHO("초나라"); |
There was a problem hiding this comment.
도메인 enum에 view 관련 로직이 들어가도 괜찮을까요? 한문으로 표기한다거나 영어로 표기한다 등의 요구사항이 생기면 어떻게 될까요?
| Position fromPosition = askFromPosition(camp, board); | ||
| Position toPosition = inputView.askToPosition(camp); |
There was a problem hiding this comment.
from을 얻을 때는 board를 통해서 검증하는 것 같은데, to를 구할때는 그냥 inputView에게 직접 물어보네요? 통일성을 주는게 좋을 것 같은데, to position에 기물이 있는지 없는지 등을 물어보는 방식을 여기에 추가할 수 있지 않을까요? 리트라이도 추가해서요
There was a problem hiding this comment.
move 내부에 있는 검증 로직을 밖으로 빼란 얘기는 아닙니다. Move 자체적인 검증 로직은 그대로 두되 from과 to를 얻어오는 방식에 통일성을 주면 좋겠다는 얘기였습니다.
|
|
||
| public void move(Position fromPosition, Position toPosition) { | ||
| if (fromPosition.equals(toPosition)) { | ||
| throw new IllegalArgumentException("[ERROR] 제자리 이동은 불가능합니다."); |
There was a problem hiding this comment.
IllegalArgumentException는 시스템적인 예외에 가깝습니다. 개발자가 뭔가 코드 작성중에 잘못된 인자를 넣는다는 식으로요. 이 부분이나 다른 부분들에서는 IllegalArgumentException보다는 비지니스 예외라는 경우가 더 클 것 같은데 따로 사용을 하는게 맞지 않을까요? 또 한가지 단점은 모든 예외들이 구분없이 IllegalArgumentException로 던지기 때문에 예외 메세지를 확인하지 않는한 구분하기 어렵고 그로인한 실수가 발생하기 쉽습니다! 이 부분 고미해보시면 좋을것 같아요!
| import org.junit.jupiter.api.Assertions; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class BoardTest { |
There was a problem hiding this comment.
보드의 경우 BoardExist라는 인터페이스 메서드들도 있는데 해당 메서드들의 테스트는 없나요?
| destination.add(move_L(from)); | ||
| destination.add(move_R(from)); | ||
| destination.add(move_D(from)); | ||
| destination.add(move_U(from)); | ||
| destination.add(move_RU(from)); | ||
| destination.add(move_RD(from)); | ||
| destination.add(move_LU(from)); | ||
| destination.add(move_LD(from)); |
There was a problem hiding this comment.
메서드 이름에 통일성이 없는 것 같습니다. Cannon에서는 moveUp 이렇게 사용됐는데 여기서는 snakecase네요?
| private Map<Position, Piece> generateGeneral(Camp camp) { | ||
| Map<Position, Piece> generalPosition = new HashMap<>(); | ||
|
|
||
| List<Position> positions = PieceLocation.GENERAL.getPositions(camp); | ||
| positions.forEach(position -> generalPosition.put(position, new General(camp))); | ||
|
|
||
| return generalPosition; | ||
| } | ||
|
|
||
| private Map<Position, Piece> generateSoldier(Camp camp) { | ||
| Map<Position, Piece> soldierPosition = new HashMap<>(); | ||
| List<Position> positions = PieceLocation.SOLDIER.getPositions(camp); | ||
| positions.forEach(position -> soldierPosition.put(position, new Soldier(camp))); | ||
|
|
||
| return soldierPosition; | ||
| } | ||
|
|
||
| private Map<Position, Piece> generateGuard(Camp camp) { | ||
| Map<Position, Piece> guardPosition = new HashMap<>(); | ||
| List<Position> positions = PieceLocation.GUARD.getPositions(camp); | ||
| positions.forEach(position -> guardPosition.put(position, new Guard(camp))); | ||
|
|
||
| return guardPosition; | ||
| } | ||
|
|
||
| private Map<Position, Piece> generateHorse(Camp camp, ElephantFormation elephantFormation) { | ||
| Map<Position, Piece> horsePosition = new HashMap<>(); | ||
| List<Position> positions = PieceLocation.HORSE.getPositions(camp, elephantFormation); | ||
| positions.forEach(position -> horsePosition.put(position, new Horse(camp))); | ||
|
|
||
| return horsePosition; | ||
| } | ||
|
|
||
| private Map<Position, Piece> generateCannon(Camp camp) { | ||
| Map<Position, Piece> cannonPosition = new HashMap<>(); | ||
| List<Position> positions = PieceLocation.CANNON.getPositions(camp); | ||
| positions.forEach(position -> cannonPosition.put(position, new Cannon(camp))); | ||
|
|
||
| return cannonPosition; | ||
| } | ||
|
|
||
| private Map<Position, Piece> generateElephant(Camp camp, ElephantFormation elephantFormation) { | ||
| Map<Position, Piece> elephantPosition = new HashMap<>(); | ||
| List<Position> positions = PieceLocation.ELEPHANT.getPositions(camp, elephantFormation); | ||
| positions.forEach(position -> elephantPosition.put(position, new Elephant(camp))); | ||
|
|
||
| return elephantPosition; | ||
| } | ||
|
|
||
| private Map<Position, Piece> generateChariot(Camp camp) { | ||
| Map<Position, Piece> chariotPosition = new HashMap<>(); | ||
| List<Position> positions = PieceLocation.CHARIOT.getPositions(camp); | ||
| positions.forEach(position -> chariotPosition.put(position, new Chariot(camp))); | ||
|
|
||
| return chariotPosition; | ||
| } |
There was a problem hiding this comment.
이 메서드들, 상,마를 제외하고는 호출자 쪽에서 타입을 파라미터로 넘겨서 생성 타입을 판단할 수 있도록 해주면 중복코드가 없어질 것 같습니다. 반복되는 코드는 지양하는게 좋은데, 이럴 경우 새로운 기물 추가 등의 요구사항이 생기면 여기다가 똑같은 메서드를 복사해서 또 만들어야하기 때문에 확장에 불편함이 있다고 할 수 있을 것 같아요!
2 participants
안녕하세요 !! 코코입니다 !☺️
우선 코드리뷰 해 주셔서 감사합니다 🙇🏻♀️
pr이 늦어지게 되어서 죄송합니다.....
미션 필수 요구사항은 기능적인 부분은 모두 구현했으나 프로그래밍 요구사항을 완벽히 수행하지 못해서 필수 요구사항 구현에 체크하지 않고 제출합니다...!
리팩토링 하면서 첫 번째 체크리스트도 체크할 수 있게 노력하겠습니다 ! 감사합니다 !!
체크 리스트
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
페어: @Eian1106