[그리디] 전서희 사다리 미션 제출합니다. #49
Conversation
|
안녕하세요~~!! 리뷰가 늦어져서 죄송합니다🥹 전체적으로 클래스명과 역할들이 잘 나눠져있어 금방 이해하기 쉬웠던 것 같아요👍저도 보면서 이렇게 쓸 수 있구나 하고 배운 부분들도 있었습니다! 부족하지만 제 의견을 추가해봤는데, 혹시라도 다른 의견이 있으시면 편하게 달아주셔서 함께 더 좋은 방향으로 개선해 나가면 좋을 것 같아요:)
우선 사다리 높이가 1일 때 이렇게 뜨더라구요!! 인원에 맞는 사다리 높이를 정해두는 것이 필요할 것 같아요. 또 플레이어 이름이 "all"일 경우도 고려해보면 좋을 것 같아요😊 추가로 개인적인 의견인데 출력 예시와 조금 다른 것 같은데 공백이 있으면 가독성이 더 좋지 않을까 싶습니다! |
|
|
||
| private boolean containsAtLeastOneTrue(List<Connection> line) { //한 층에 연결이 하나라도 있긴 해야함 | ||
| for (Connection c : line) { | ||
| if (c.hasRight()) { |
| } | ||
|
|
||
| public List<Connection> getConnections() { | ||
| return Collections.unmodifiableList(connections); |
There was a problem hiding this comment.
전체 클래스에서 List.copyOf() 와Collections.unmodifiableList 를 사용하셨는데, 이 두 방법을 따로 사용하신 기준이 있는지 궁금합니다!
There was a problem hiding this comment.
getter처럼 내부의 리스트를 반환할 때 unmodifiableList로 사용하였습니다!
더 찾아보니 unmodifiavbleList는 원본 컬렉션에서 변경이 일어나면 영향을 받지만 copyOf는 수정을 반영하지 않는다고 하네요..좀 더 유의해서 사용해야할 것 같네여
|
|
||
| Map<PlayerName, PrizeName> resultMap = new LinkedHashMap<>(); | ||
| for (int i = 0; i < playerNames.size(); i++) { | ||
| resultMap.put(playerNames.get(i), prizeNames.get(endPositions.get(i))); |
There was a problem hiding this comment.
이 부분은 저도 이번에 사다리 미션을 하면서 리뷰를 받아 알게된 점입니다!
- 현재 PlayerNames와 PrizeNames는 List 컬렉션을 외부에 노출시키고 있습니다. 디미터의 법칙을 고민해보면 좋을 것 같아요👍
- 위 코드에서 for 반복문을 사용하여 Map을 생성하고 있는데, 이는 상태를 변경하는 부수 효과를 발생시킬 수 있다고 합니다. 이에 대해 더 알아보고 함수형 프로그래밍을 적용시키면 좋을 것 같습니다:)
There was a problem hiding this comment.
조건사항중 3개 이상 인스턴스 변수를 가지는 클래스를 만들지 말라는 조건때문에.. GameInformation 클래스를 따로 만들었습니다.
저는 개인적으로 PlayNames와 PrizeNames 를 묶는 역할만 하고 있는데, 실제 도메인 로직에 대한 책임이 부족하다고 생각이 드는 것 같아요! 만약 이 이유로 클래스를 따로 만든 것이라면, 생성자로 받는 것보다 메서드로 받아서 처리할 수 있지 않을까 싶어요 혹시 다른 의견 있다면 편하게 말씀해주세요😊
| void 사다리를_통해_정상적으로_결과_계산된다() { | ||
| PlayerNames playerNames = new PlayerNames(List.of(new PlayerName("a"), new PlayerName("b"))); | ||
| PrizeNames prizeNames = new PrizeNames(List.of(new PrizeName("1"), new PrizeName("2"))); | ||
| Ladder ladder = new Ladder(0, 2); //그냥 그대로 매칭 |
There was a problem hiding this comment.
개인적으로 저는 저번 로또 미션 때도 이런 구조에 대해 고민을 많이 했던 기억이 있어요🤔 이 코드에서 사다리 높이가 0인 경우만으로 결과를 검증하고 있는데, 정말 이 코드로 사다리 로직이 제대로 동작하는지 확인할 수 있을까? 하는 생각이 들게되는 것 같아요!
이번 미션에서도 사다리를 랜덤하게 생성하고 있다 보니, 결과 예측이 어려운 테스트들이 많아 저는 테스트 시 사다리를 직접 제어 가능한 방식으로 생성할 수 있어야 한다고 생각했습니다. 이런 이유로 BooleanValueGenerator 와 같은 인터페이스를 두고 테스트 하위 항목에 TestLadderGenerator 같은 구현체를 따로 만들어 테스트를 진행했는데요! 서희님은 어떻게 생각하시는지 궁금합니다
There was a problem hiding this comment.
현재 구조에서는 Ladder가 내부에서 BooleanValueGenerator를 통해 임의로 생성되기 때문에, 테스트마다 다른 결과가 나올 수 있고, 이로인해 정확한 테스트가 어렵겠네요.. 말씀하신 방식처럼 BooleanValueGenerator를 외부에서 주입하는 방식으로 테스트를 수정해보겠습니다!
src/test/java/ConnectionTest.java
Outdated
There was a problem hiding this comment.
테스트 코드에서도 원래 클래스의 패키지 구조를 따르면 좋을 것 같습니다👍
|
안녕하세요~! 리뷰달아주신 거 토대로 수정했어요! |
|
답장이 늦어 죄송합니다!! 반영을 잘 해주셨네요👍 리뷰 다는게 익숙하지 않아서 피드백이 좀 부족했던 것 같네요..! 앞에 달았듯이 시간 되실 때 함수형 인터페이스를 써보는 것도 좋을 것 같아요😊 |
| } | ||
|
|
||
| public record IndexedPlayerName(int index, PlayerName name) { | ||
| } |
There was a problem hiding this comment.
domain 패키지 안에서 하위 패키지로 나누는 방법도 나쁘지 않을 것 같아요!!
| return line.stream().anyMatch(Connection::hasRight); | ||
| } | ||
|
|
||
| public int move(int index) { |
There was a problem hiding this comment.
저도 리뷰받은 내용인데 공유해 보겠습니다 이전 스터디 시간에 함수형 인터페이스에 대해 배운 적이 있었죠!! 저는 처음에 이걸 어떻게 적용해야 될지 감이 안 잡혀 못썼었는데 마지막 자바 미션인 만큼 사용해 보는 게 의미 있겠다 싶어서 리팩토링 과정에서 추가해 봤습니다. 시간이 별로 남지않아서 따로 여유될 때 한번 해보시는 것도 좋을 것 같아요!!
저는 이동 방향에 대한 전략을 분리하는 방식으로 적용을 했습니다.
각 방향(RIGHT, LEFT, STAY)에 대해
1. 이동 가능 여부를 판단하는 전략 함수
2. 새로운 포지션을 반환하는 메서드
를 ENUM에 넣어, 이동 판단과 책임을 처리해 주었습니다.
이렇게 구현하면 새로운 이동 전략이 추가될 때, 기존의 도메인 코드를 수정하지 않아도 확장이 편하고 또 외부 상태를 변경하지 않기 때문에 불변성과 부수 효과도 막을 수 있습니다😊
| } | ||
| } | ||
|
|
||
| private static void validatePrizeInput(List<String> prizes, int expectedSize) { |
There was a problem hiding this comment.
우선 저는 검증로직을 도메인에 주로 써왔어서 보고 이런 방식도 있구나해서 알게된 것 같아요. 근데 따로 InputView에 둔 이유가 있는지 도메인과 뷰로 나눈 기준이 궁금합니다!
There was a problem hiding this comment.
단순 입력의 유효성이라서 사용자 인터페이스 레벨에서 발생하는 문제라고 생각했습니다! 입력받을 때 바로 필터링되게끔 하고 도메인에서는 검증이 이미 된 값으로만 다룰려고 하였습니다!
|
안녕하세요~! 이번 미션은 제가 뭔가 정신없이 했던 것 같아 많이 아쉬움도 남지만, 제 코드 열심히 리뷰해주셔서 너무 감사드립니다!! |
안녕하세요!! 미션 잘 부탁드립니당~~
이번 미션을 하면서 사다리 구현 자체를 어떻게 할 것인가에 대한 고민이 제일 많았던 것 같습니다.
사다리 구현은 Ladder에서 높이 만큼 LadderRow를 생성하고 LadderRow는 Connection리스트를 생성하도록 하였고, 이 Connection 리스트에서 오른쪽과 연결되어 있는 지 여부를 나타내도록 하였습니다.
즉 Ladder <- List
LadderRow <- List 구조이고,
예를 들어 참가자가 4명이고 그중 한 층에서 List이 [disconnected, connected,disconnected] 인경우 해당 층은 참가자 2-3만 연결된 상태임을 의미합니다.
조건사항중 3개 이상 인스턴스 변수를 가지는 클래스를 만들지 말라는 조건때문에.. GameInformation 클래스를 따로 만들었습니다. PlayerResults와 차별점을 두기 위해 이 클래스에서는 입력된 데이터 자체를 관리하도록 하였고 PlayerResults는 결과 계산 이후의 결과만을 관리 및 계산 결과를 저장과 조회 기능만 담당하게 하였습니다. 결과를 계산하는 클래스는 ResultCalculator로 따로 분리 하였습니다.
요구사항을 지키다보니 조금 복잡해지고 메서드가 많아진 경향이 있는 것같은데..보시다가 좀 더 간결하게 나타내도 괜찮겠다 싶은 부분도 말씀해주세요!
이외에도 이해가 안가거나, 다른 방식으로 구현해봐도 좋겠다 싶은 부분 등 편하게 말씀해주세요!
감사합니다!!