[송지은] 사다리타기 제출합니다. #38
Conversation
There was a problem hiding this comment.
@hafnium1923 👋🏻
서로가 바쁘다 보니까 쏟고 싶은 열정은 큰데 다른 할 일들도 있다보니 어쩔 수 없음이 느껴져 아쉽군요. 하루가 48시간이었으면 일 다 끝내고 쉴 텐데요~
모든 클래스 인스턴스 변수 3개이하로 가져가기 위해
그만큼 관심사 분리 측면에 좀 신경을 썼어요.
오 보니까 하나의 관심사만 담당하도록 엔티티들을 작게 쪼개셨더라고요, 그래서 이 요구사항을 지키실 수 있지 않았나 싶어요. 저는 한 곳에서 인스턴스 변수 3개 이하를 못 쓰겠더라고요, 그래서 부득이하게 4개를 썼는데, 이 부분은 제가 조언을 받아보고 싶군요
컬렉션 및 클래스를 래핑하는 작업은 전반적으로 잘 해주신 것 같아서 별도로 드릴만한 리뷰는 없었고, 그 대신 기능 측면과 이외 약간의 마이너한 코멘트들을 달아보았어요.
수고하셨습니다, 마지막까지 화이팅해보죠 ✏️
| private final List<Position> positions; | ||
|
|
||
| public LadderResult(List<Position> positions) { | ||
| this.positions = positions; |
There was a problem hiding this comment.
별도의 보호 조치 없이 얕은 복사가 일어날 만한 부분의 코드처럼 보여요! LadderResult의 인스턴스를 만들기 위해 positions를 주입해주었는데 이후 외부에서 positions가 바뀔 경우 이 인스턴스 내의 positions 또한 바뀌는 현상은 아마도 원치 않으실 것 같아요.
ArrayList<>()의 인자로 넣는 방법 또는 List.of() 등을 활용해 방어적 복사 가 되도록 개선해 보시는 것에 대해서는 어떻게 생각하세요? 마침 블로그도 루루가 아는 사람의 블로그입니다 😈
| public class LadderController { | ||
| public static void main(String[] args) { | ||
| InputView inputView = new InputView(); | ||
| OutputView outputView = new OutputView(); | ||
| Players players = inputView.askPlayers(); | ||
| Prizes prizes = inputView.askPrizes(); | ||
| LadderHeight height = inputView.askHeight(); | ||
| LadderWidth width = new LadderWidth(players.size()); | ||
| LadderGame ladderGame = new LadderGame(players, prizes, new Ladder(height, width)); | ||
| outputView.printLadderGameBoard(ladderGame); | ||
| GameResult gameResult = ladderGame.play(); | ||
| String query = inputView.askResultName(); | ||
| if (query.equals("all")) { | ||
| outputView.printResultAll(gameResult); | ||
| return; | ||
| } | ||
| outputView.printResultSingle(gameResult.getResultByPlayerName(query).getPrize()); | ||
| } | ||
| } |
There was a problem hiding this comment.
- 전반적으로 덩치가 꽤 큰 Controller라는 생각이 들었는데, 관심사나 상황별로 분리해 보시는 것은 어떨까요?
- 사용자가 잘못된 값을 입력한 경우 다시 입력받게 하는 것까지는 선택사항으로 볼 수 있어보이지만, 적어도 예외가 발생했을 경우 사용자에게 전용 예외 메시지를 보여주고 종료하면 좋을 것 같다는 생각도 들었는데, 어떻게 생각하세요?
- 사용자가 잘못된 값을 입력한 타이밍과 예외가 발생하는 시점의 타이밍도 서로 다른 것 같아요! 입력받는 시점과 컬렉션을 선언하는 타이밍이 다를 경우에 생길 수 있는 문제로 보이는데, 이건 어떻게 해결해 볼 수 있을까요? 저도 자주 고민하는 토픽이네요
|
|
||
| public LadderWidth(int value) { | ||
| if (value < 2) { | ||
| throw new IllegalArgumentException("사다리의 넓이는 2 이상이어야 합니다."); |
There was a problem hiding this comment.
넓이 라고 하니까 뭔가 사다리의 가로 길이와 세로 길이를 곱한 개념처럼 느껴졌는데, LadderWidth의 width는 주로 가로 길이를 의미하는 만큼 이에 맞는 예외 메시지를 작성해 보시는 것은 어떨까요?
| public Position move(Position position) { | ||
| int current = position.getValue(); | ||
| if (canMoveLeft(current)) { | ||
| return new Position(current - 1); | ||
| } | ||
| if (canMoveRight(current)) { | ||
| return new Position(current + 1); | ||
| } | ||
| return position; | ||
| } | ||
|
|
||
| private boolean canMoveLeft(int current) { | ||
| return current > 0 && bridges.get(current - 1) == Bridge.CONNECTED; | ||
| } | ||
|
|
||
| private boolean canMoveRight(int current) { | ||
| int columnCount = bridges.size() + 1; | ||
| return current < columnCount - 1 && bridges.get(current) == Bridge.CONNECTED; | ||
| } |
There was a problem hiding this comment.
사다리를 해석하고 연결 여부에 따라 아래로 따라가는 식으로 시뮬레이션 하는 코드군요? 직접 살펴봤는데 범위를 벗어날 만한 상황도 잘 처리되어 있네요
역시 일일이 따라가는 식으로 시뮬레이션 하는 방법이 그나마 직관적인 것 같아요, 다른 방법은 정말 안 떠오르더라고요
| public class InputView { | ||
| private final Scanner scanner; | ||
|
|
||
| public InputView() { | ||
| scanner = new Scanner(System.in); | ||
| } | ||
|
|
||
| public Players askPlayers() { | ||
| System.out.println("참여할 사람 이름을 입력하세요. (이름은 쉼표(,)로 구분하세요)"); | ||
| String input = scanner.nextLine(); | ||
| List<Player> list = new ArrayList<>(); | ||
| for (String name : input.split(",")) { | ||
| list.add(new Player(new Name(name.trim()))); | ||
| } | ||
| return new Players(list); | ||
| } | ||
|
|
||
| public Prizes askPrizes() { | ||
| System.out.println("실행 결과를 입력하세요. (결과는 쉼표(,)로 구분하세요)"); | ||
| String input = scanner.nextLine(); | ||
| List<Prize> list = new ArrayList<>(); | ||
| for (String prize : input.split(",")) { | ||
| list.add(new Prize(prize.trim())); | ||
| } | ||
| return new Prizes(list); | ||
| } | ||
|
|
||
| public LadderHeight askHeight() { | ||
| System.out.println("최대 사다리 높이는 몇 개인가요?"); | ||
| int input = Integer.parseInt(scanner.nextLine()); | ||
| return new LadderHeight(input); | ||
| } | ||
|
|
||
| public String askResultName() { | ||
| System.out.println("결과를 보고 싶은 사람은?"); | ||
| return scanner.nextLine().trim(); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 클래스는 멤버 변수가 scanner 뿐이고, 내부의 메서드들 모두 scanner 외에 다른 의존하는 멤버 변수가 없는 상태이네요? 여기에서는 Scanner가 각 인스턴스마다 독자적으로 존재할 필요 없이 static으로 선언된 채로 존재해도 문제가 없어보여요.
또한, 이에 더해 Scanner가 private static 멤버 변수가 된다면 InputView 클래스의 모든 메서드들도 static 이 될 수 있을 것 같아요, 그러면 사용을 위해 매번 인스턴스를 생성하지 않아도 될 것 같아보이고요! 어떻게 생각하시나요?
혹시 의도적으로 static 이 아닌 메서드들로 두신 거라면, 의견도 들어보고 싶습니다!
| public Name(String value) { | ||
| if (value.length() > 5) { | ||
| throw new IllegalArgumentException("이름은 최대 5글자여야 합니다."); | ||
| } | ||
| this.value = value; | ||
| } |
There was a problem hiding this comment.
이런 상황과 같이 이름이 0글자인 경우 통과할 수도 있으니 수정해 주시면 좋을 것 같아요
| public class Player { | ||
| private final Name name; | ||
|
|
||
| public Player(Name name) { | ||
| this.name = name; | ||
| } | ||
|
|
||
| public Name getName() { | ||
| return name; | ||
| } | ||
| } |
There was a problem hiding this comment.
마지막 부분에서 결과 조회할 때 all을 치면 전체 결과가 떠요. 그러면 플레이어명이 all일 경우 이 명령어와 겹칠 수 있으니 별도의 처리가 필요할 것 같아요
이미 글자 수 확인 로직은 하위 클래스인 Name에서 하고 있으니 여기에서 별도로 예외 처리 해보시는 것은 어떨까요?
| if (prizes == null || prizes.isEmpty()) { | ||
| throw new IllegalArgumentException("결과는 비어 있을 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
오, null은 이번에 처음 보네요? 혹시 List<Prize>에 null이 들어갈 수도 있어서 이렇게 조건을 적어주신 걸까요?
아아안녀어어엉 하세요... 2주나 지각한(ㅠㅠ) 지각생 등장입니다.
이번미션은 약간 간단한 어플리케이션 만드는 느낌으로 만들어서 재밌었어요.
다만 시간과 체력이 없는 관계로 5단계 테스트는 못했습니다ㅏ(아마 아예 못할 것 같아서 이부분은 리뷰에서 패스 부탁드립니다)
모든 클래스 인스턴스 변수 3개이하로 가져가기 위해 많은 클래스로 세분화 하였고, 그만큼 관심사 분리 측면에 좀 신경을 썼어요.
java 스터디의 마지막 미션이네요..! 여러가지 사정상 예정된 기간보다는 많이 오버 되었지만, 그래도 완벽하게 끝낼 수 있어서 좋아요.
언제나 저를 기다려준 토끼에게 고맙습니다.
마지막 리뷰 잘 부탁드려요~!
!