[그리디] 하수한 사다리 미션 제출합니다. #83
Conversation
There was a problem hiding this comment.
안녕하세요 수한님! 미션 하시느라 고생 많으셨어요😃
앞으로 함께 많은 이야기 나눌 수 있으면 좋겠습니다. 잘 부탁드립니다!
우선 PR에 각 클래스별로 코드의 구조와 흐름이 꼼꼼하게 정리되어 있어서 읽기 편했어요.
추가로, 저는 개인적으로 README에 코드에서 사용되는 용어나 클래스 또는 변수명 등을 정리해두면 다른 분들이 이해하기 더 쉽더라구요. 나중에 한 번 적용해보셔도 좋을 것 같습니다!
로직 전반적으로 객체가 잘 분리되어 있고 VO도 적절하게 활용하신 점이 깔끔하니 아주 좋았습니다!
입력값에 대한 검증 로직이 조금 더 보강되면 더 견고한 코드가 될 것 같아요. 미션 요구사항에 명시되지 않았더라도, 수한님께서 필요하다고 판단되는 검증은 자유롭게 추가해보셔도 좋을 것 같습니다 :)
이번 리뷰에서는 Controller의 흐름을 따라가며, 비즈니스 로직을 제외한 부분을 중심으로 피드백을 남겨보았습니다. 다음 리뷰에서는 비즈니스 로직을 위주로 읽어보겠습니다!
🤔 질문사항 답변
아래는 질문사항에 대한 답변입니다
- 이번 미션에서는 함수형 프로그래밍 문법을 적극적으로 사용해보았는데, 사용 과정에서 로직이 복잡해질수록 depth 자체는 감소할 수 있어도 가독성 측면에서는 오히려 알아보기가 힘들어진다는 느낌을 받았습니다. (e.g., LadderController::handleOutcomeQuery) 따라서 이러한 부분에 대해 어느 정도 trade-off가 존재한다고 생각하는데, 해윤님은 어떠한 생각을 가지고 계신지 궁금합니다..!
새로운 방식을 시도해보신 점이 정말 좋네요👍
저는 함수형 프로그래밍 방식을 도입했더니 코드의 가독성이 좋아지지 않았다면, 다른 방법도 고안해볼 것 같아요! 예를 들어 depth를 줄이기 위해, stream등의 문법을 사용했는데 가독성이 안좋아졌다면, 다른 방법으로 depth를 줄일 수 있는 방법을 생각해보는거죠.
또한 어떤 로직이 너무 길고 복잡해보인다 하면, 한 메서드 또는 절이 하나의 역할만 하는가? 를 먼저 고민해보면 좋을 것 같아요. 하나의 절이 많은 역할을 수행하게되면 읽기 힘든 복잡한 코드가 되기 쉬우니까요!
말씀해주신 LadderController의 handleOutcomeQuery 메서드를 보도록하겠습니다. 해당 메서드는 입력 + 반복 제어 + 결과 조회 + 출력의 역할을 하고있어요. 근데 네가지 기능을 하나의 stream절에서 실행하고 있으니 딱 봤을 때 흐름이 눈에 쉽게 들어오지 않는 것 같아요. 이러한 점들을 고려하여 리팩토링 진행해보시면 좋을 것 같습니다.
결론은 읽기 쉬운 코드를 작성하는 것은 매우 중요하기 때문에, 가독성에서의 문제를 느꼈다면 사용하지 말자(=대안을 찾자)! 였습니다
- 이번 미션의 경우에도 depth가 2를 넘지 않게 구현하도록 하는 조건이 명시되어 있었습니다. LadderFactory::createConnections() 메소드의 경우 고민을 해보았으나 도무지 depth를 여기서 더 줄일 방법이 떠오르지 않았습니다 ㅠ 혹시 해윤님은 이와 관련해서 다른 아이디어가 있으신지 궁금합니다!
생각나는 방법은
- Early return
for (int i = 0; i < width - 1; i++) {
if (!shouldConnect(random)) continue;
connections.add(new Connection(i, i + 1));
i += 1; // skip the right next line
}
'어떤 조건을 만족한다면 ~~해라' 가 아닌, 기본적으로 ~~를 하지만 '어떤 조건을 만족하지 않는다면 그 전에 반환 또는 break 또는 continue'를 하는 방법입니다.
조금 억지같지만 depth는 1로 만들 수 있을 것 같아요!
- 메서드 분리
if (shouldConnect(random)) {
connections.add(new Connection(i, i + 1));
i += 1; // skip the right next line
}
가장 많이 사용해 보셨을 방법인데요, 위 로직을 메서드로 분리하면 depth가 줄어들게 됩니다. 이렇게 되면 반복과, 연결 생성을 별도의 메서드로 분리할 수 있다는 장점도 있겠네요!
| public class Participant { | ||
| public static final int PARTICIPANT_NAME_MAX_LENGTH = 5; | ||
| private final String name; | ||
|
|
||
| public Participant(String name) { | ||
| if (name.length() > PARTICIPANT_NAME_MAX_LENGTH) { | ||
| throw new IllegalArgumentException("참가자의 이름은 최대 " + PARTICIPANT_NAME_MAX_LENGTH + "자만 가능합니다."); | ||
| } | ||
|
|
||
| this.name = name; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
| } |
There was a problem hiding this comment.
수정할 필요는 없지만 같은 단순한 값을 나타내는 객체를 VO라고 하는데요, 값 객체는 class가아닌 record로 사용해도 좋습니다!
record는 모든 필드값이 불변 객체로 선언되고, equals(), hashCode(), toString()등의 코드가 자동 생성된다는 장점이 있습니다.
이러한 개념이 있다고 알고 넘어가면 좋을 것 같아요~
| // method chaining으로 구성하려고 하였으나 Input 검증으로 인해 각각 따로 받음 | ||
| GameConfigurationBuilder builder = new GameConfigurationBuilder(); | ||
|
|
||
| List<Participant> participants = InputView.getParticipants().stream().map(Participant::new).toList(); |
There was a problem hiding this comment.
입력받은 String을 List로 변환하는 과정을 두 단계로 나누어
string을 List으로 변환하는 로직은 inputView에 위치하고,
List을 List와 같이 객체들의 리스트로 저장하는 로직은 컨트롤러에 작성하셨네요!
해당 로직을 모두 controller에서 진행하지 않고, 단계 별로 계층을 다르게 한 이유가 궁금합니다!
There was a problem hiding this comment.
추가로 participant 입력에 대한 검증 로직이 없어
,또는,,(,만으로 이루어진 문자열) 입력한 경우- 공백 혹은 아무것도 입력하지 않은 경우
에 예외를 던지지 않고 다음 단계 입력 로직이 실행되네요! 적절한 위치에 검증 로직을 추가해 위와 같은 경우에도 예외문을 던지면 좋을 것 같은데, 어디에 위치시키면 좋을 것 같나요?
| public static List<String> getParticipants() { | ||
| System.out.println("\n참여할 사람 이름을 입력하세요. (이름은 " + INPUT_DELIMITER + "로 구분하세요)"); | ||
|
|
||
| return Arrays.stream(scanner.next().split(INPUT_DELIMITER)).toList(); |
There was a problem hiding this comment.
저라면 해당 로직을 이렇게 작성할 것 같아요!
return Arrays.asList(scanner.nextLine().split(INPUT_DELIMITER));
저는 입력을 받을 때 한 줄씩 받기 위해 scanner.nextLine으로 받는 편인데 scanner.next를 선택한 이유가 있을까요?
또한 고정된 리스트인 Arrays.asList()대신 Stream.toList()를 사용한 특별한 이유가 있다면 궁금합니다!
| public GameConfigurationBuilder participants(List<Participant> participants) { | ||
| if (participants == null || participants.isEmpty()) { | ||
| throw new IllegalArgumentException("참가자 목록은 비어 있을 수 없습니다."); | ||
| } | ||
|
|
||
| this.participants = participants; | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
현재 입력 터미널에서 엔터만 입력하여도 해당 로직이 오류를 감지하지 못하고있는데, 어디를 수정해야 할까요🤔
| public class LadderController { | ||
| public void run() { | ||
| try { | ||
| GameConfiguration configuration = readConfiguration(); |
There was a problem hiding this comment.
개별 값(participants, outcomes, width, height)을 바로 넘기지 않고, GameConfiguration 객체를 통해 입력값을 한 번에 전달하도록 설계하신 이유가 궁금합니다!
| Stream.generate(InputView::getParticipantForResult) | ||
| .takeWhile((input) -> !input.equals("all")) | ||
| .forEach((input) -> result.getResults().keySet().stream() | ||
| .filter((participant) -> participant.getName().equals(input)) | ||
| .findFirst() | ||
| .ifPresentOrElse( | ||
| (target) -> OutputView.printGameResultOf(target, result), | ||
| () -> System.out.println("존재하지 않는 참가자입니다.") | ||
| ) | ||
| ); |
There was a problem hiding this comment.
하나의 stream에서 너무 많은 역할을 하고 있는 것 같아요! 로직을 분리해보면 좋을 것 같습니다~
|
|
||
| public class InputView { | ||
| private static final Scanner scanner = new Scanner(System.in); | ||
| private static final String INPUT_DELIMITER = ","; |
There was a problem hiding this comment.
구분자를 view계층에도 상수화시켜 관리 포인트를 줄인점이 매우 좋네요~
2 participants
해윤님 안녕하세요! 리뷰어로써는 처음 뵙는 것 같은데, 이번 사다리 미션도 잘 부탁드리겠습니다 🙃
아래는 현재 프로젝트 구조입니다:
애플리케이션 실행 흐름은 다음과 같습니다:
InputView)GameConfigurationBuilder로 넘겨주어GameConfiguration생성과 동시에 값 검증을 수행합니다. (LadderController::readConfiguration())Game객체를 생성하여 게임을 실행합니다.다음은 사다리 생성 및 이동 로직에 대한 부가 설명입니다:
사다리 생성
사다리 이동
아래는 구현 과정에서 발생한 질문 사항입니다:
이번 미션에서는 함수형 프로그래밍 문법을 적극적으로 사용해보았는데, 사용 과정에서 로직이 복잡해질수록 depth 자체는 감소할 수 있어도 가독성 측면에서는 오히려 알아보기가 힘들어진다는 느낌을 받았습니다. (e.g.,
LadderController::handleOutcomeQuery)따라서 이러한 부분에 대해 어느 정도 trade-off가 존재한다고 생각하는데, 해윤님은 어떠한 생각을 가지고 계신지 궁금합니다..!
이번 미션의 경우에도 depth가 2를 넘지 않게 구현하도록 하는 조건이 명시되어 있었습니다.
LadderFactory::createConnections()메소드의 경우 고민을 해보았으나 도무지 depth를 여기서 더 줄일 방법이 떠오르지 않았습니다 ㅠ 혹시 해윤님은 이와 관련해서 다른 아이디어가 있으신지 궁금합니다!작성하다 보니 내용이 길어졌는데, 혹시 코드 관련해서 부가적인 설명이 필요한 부분이 있으시면 언제든지 말씀해주세요!
긴 글 읽어주셔서 감사합니다~!