[사다리] 권장순 미션 제출합니다. #51
Conversation
There was a problem hiding this comment.
다시 뵙습니다 장순님! 확실히 지난 미션 처음 PR 올렸을 때보다 훨씬 코드가 슬림해지고 깔끔한 것 같아요. 물론 그만큼 클래스 하나하나가 커진 부분이 있는데, 이건 리뷰 이후 리팩토링할 때 필요한 만큼 분리해나가면서 고쳐나가도록 해요!
LadderBoard
LadderBoad가 커보이지만 만드는 과정에서 필요한 기능들을 하고 있을 뿐이고, 여러 책임을 가지고 있는 것이 아니기 때문에 이상하지 않은 것 같아요! 너무 한 클래스가 슬림할 필요는 없습니다! 클래스 분리의 기준은 책임이에요!
검증과 View
이미 문제점을 생각하셨군요. InputView가 본인이 하는 역할이 없습니다. 또한 Validator+Parser를 하나로 구성하신 것 같은데, 적절한 책임이 아닌 것 같아요. 이 부분은 코멘트로 남겨두었어요! 그리고 View를 분리하고 싶다고 하셨고 Parser는 분리를 하셨는데 왜 View가 파싱에 대한 책임을 가져서는 안되는걸까요? 또한 본문 고민에도 기존에 제가 작성하던 코드들에 비해서 OutputView에 로직이 담겨 있다고 느껴졌는데요 라고 적어주셨는데 왜 View는 꼭 날씬해야 한다고 생각하시는지 궁금합니다. 입출력을 담당하는 클래스가 View라면 View가 입출력에 관련된 행위로 커지는 것은 자연스러운 것이 아닐까요?
끝으로, 응집도가 높은 코드를 만드는 것은 중요합니다만, (지금 그렇다는 것은 아니고) 한 클래스에 로직을 다 때려박고 응집도가 높다! 라고 하는 것은 경계할 필요가 있습니다. 기존 미션에서 불필요한 클래스 분리가 많고 제 역할을 하지 못하는 클래스가 많다는 부분을 지적한 것이지 단순히 한 클래스에 모든 로직을 넣으라는 것은 아니였어요! 클래스의 적절한 분리는 필요합니다. 지금 코드에서도 클래스 분리가 들어가면 좀 더 좋을만한 부분이 있을지 본인만의 기준을 토대로 되짚어보고 적절한 리팩터링을 해보면 좋을 것 같아요.
그리고 5단계 함수형 프로그래밍 적용도 잊지 말아주세요!
| public class InputView { | ||
|
|
||
| private static final Scanner scanner = new Scanner(System.in); | ||
|
|
||
| public String readParticipantNames() { | ||
| return readLine(); | ||
| } | ||
|
|
||
| public String readResultLabels() { | ||
| return readLine(); | ||
| } | ||
|
|
||
| public String readLadderHeight() { | ||
| return readLine(); | ||
| } | ||
|
|
||
| public String readResultRequest() { | ||
| return readLine(); | ||
| } | ||
|
|
||
| private String readLine() { | ||
| return scanner.nextLine().trim(); | ||
| } | ||
| } |
There was a problem hiding this comment.
기본적인 파싱은 view에서 해도 괜찮을 것 같아요! 예를 들면 ParticipantNames를 String의 List로 바꿔준다든지, Request 객체로 파싱해준다든지요. 만약 모든 read가 readLine 한 줄 호출이 끝이라면, 그냥 readLine을 public으로 열고 Controller가 그걸 호출하면 그만일테니까요!
There was a problem hiding this comment.
공감합니다! 🙌
본문에서 말씀해주신 내용을 다시 곱씹어보니, View에서 기본적인 파싱까지 처리해주는 것이 자연스럽고 책임의 흐름에도 어긋나지 않는 방향이라는 생각이 들었어요. 단순히 readLine()만 호출하는 구조라면 오히려 View의 역할이 너무 축소된 게 아닌가 하는 생각도 들었고요!
말씀 주신대로 List으로의 변환이나, 요청 객체로 파싱하는 정도까지 View에서 담당하도록 한번 적용해보겠습니다 :)
| public static LadderBuildResponse build(LadderBuildRequest request) { | ||
| int participantCount = request.columnCount(); | ||
| int ladderHeight = request.rowCount(); | ||
| int centerColumnIndex = (participantCount - 1) / 2; | ||
|
|
||
| List<BridgeLine> bridgeLines = generateValidBridgeLines(participantCount, ladderHeight, centerColumnIndex); | ||
| return new LadderBuildResponse(participantCount, bridgeLines); | ||
| } |
There was a problem hiding this comment.
Domain과 DTO의 의존관계는 어떻게 되어야 할까요? Domain이 DTO를 알아도 괜찮을까요?
There was a problem hiding this comment.
생각해보니 DTO는 뷰처럼 변경 가능성이 높은 계층이라, 도메인에서 직접 의존하는 건 바람직하지 않은 관계라고 생각이 돼요!
도메인은 그저 필요한 값만 전달받으면 되는데, DTO의 구조를 직접 알고 있게 되면 도메인의 순수성과 역할이 흐려질 수 있다는 것도 다시 한번 느꼈습니다.
그래서 말씀해주신 대로 도메인이 DTO를 알지 않고, 메시지로만 협력할 수 있도록 의존 방향을 개선해보겠습니다!✨
| private static List<BridgeLine> generateValidBridgeLines(int participantCount, int ladderHeight, int centerColumnIndex) { | ||
| while (true) { | ||
| List<BridgeLine> candidateBridgeLines = tryGenerateBridgeLines(participantCount, ladderHeight, centerColumnIndex); | ||
| if (candidateBridgeLines != null) return candidateBridgeLines; | ||
| } | ||
| } |
There was a problem hiding this comment.
아래 tryGenerateBridgeLines, generateConnectionStates 까지 종합적으로 봤을 때, participantCount가 1이라면 무한루프가 발생할 것 같네요! 장순님께서 구상한 도메인 규칙 상 participantCount는 1이 가능한데 말이죠!
There was a problem hiding this comment.
optional) 또한 기본적으로 while(true)를 통해 무한으로 돌고 있는데요, 기본적인 사다리 연결 방식은 랜덤인데 만약 정말 운이 없어서 계속 연결이 안되는 상황도 고려해본다면 어떨까요?
There was a problem hiding this comment.
오찌의 꼼꼼한 피드백 덕분에 중요한 부분을 놓치고 있었다는 걸 알게 되었어요! 🙏
우선 말씀해 주신 것처럼, 현재 구조에서는 participantCount == 1일 때 generateValidBridgeLines() 메서드에서 무한 루프가 발생할 수 있겠네요. 구체적으로 요구사항을 생각하지 않고 구현을 진행한 것 같네요! 말씀처럼 사다리라는 구조 자체가 두 명 이상이어야 의미 있는 형태라는 점을 완전히 간과하고 있었습니다. 해당 조건에 대한 명시적인 방어 로직을 추가해 무의미한 루프를 방지해 볼게요!.
또한 while (true) 구조에 대해서도 짚어주신 부분, 정말 감사드립니다! 랜덤성 기반 생성 로직이기 때문에 최악의 경우 조건을 만족하지 못한 채 반복될 가능성도 분명 존재하는데, 이를 제한 없이 두는 건 예측 불가능한 상황을 야기할 수 있다는 걸 다시금 느꼈어요. 말씀해 주신 MAX_ATTEMPTS 제한을 도입해 실패 시 적절한 예외를 던지도록 개선해 보겠습니다!
요구사항 분석!
최악의 시나리오 생각하기!
There was a problem hiding this comment.
넵 사실 요런 부분을 안놓치는 팁은,
- 테스트 코드에만 의존하지 않고, 실제 애플리케이션을 돌려서 최대한 비정상적, 혹은 정상적이더라도 극단적인 케이스를 입력해서 체크해보세요
- 요즘은 AI가 코드 및 요구사항 분석도 잘해줘서, AI에게 발생할 수 있는 문제점을 분석해달라는 프롬프트를 작성해보세요
이정도가 있을 것 같아요!
| BridgeLine line = new BridgeLine(connectionStates); | ||
| generatedBridgeLines.add(line); | ||
|
|
||
| isCenterBridgePresent |= isConnectedAtCenter(line, centerColumnIndex); |
There was a problem hiding this comment.
조금 멋을 부려보고 싶었던 표현이었는데… 말씀 듣고 보니 오히려 가독성을 해치는 꾸밈이었던 것 같네요 😅
비트 연산자가 간결하긴 하지만, 직관적으로 읽히지 않으면 오히려 혼란을 줄 수 있다는 점에 공감합니다!
말씀 주신 부분은 더 명확한 방식으로 가독성을 우선해 개선해보겠습니다. 감사합니다! 🙏
There was a problem hiding this comment.
각종 멋부리는 연산자(대표적으로 삼항 연산자)들은 원래 처음 접하면 되게 신박해보여서 몇번 써먹어보긴 하는데, 사실 실무에서는 눈에 바로 들어오지 않는 코드는 협업에 방해가 될 수 있어서 자제하는 편이긴 합니다!
| public class BridgeLine { | ||
|
|
||
| private final List<Boolean> horizontalConnections; | ||
|
|
||
| public BridgeLine(List<Boolean> horizontalConnections) { | ||
| this.horizontalConnections = horizontalConnections; | ||
| } | ||
|
|
||
| public boolean isConnectedAt(int index) { | ||
| return horizontalConnections.get(index); | ||
| } | ||
|
|
||
| public int width() { | ||
| return horizontalConnections.size(); | ||
| } | ||
|
|
||
| public List<Boolean> getConnections() { | ||
| return horizontalConnections; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 클래스는 사다리의 가로 줄 1개를 나타내는건가요? 그렇다고 보기에는 뭔가 조금 허전한 것 같은데, 추가적인 리뷰를 위해 이 클래스의 정의와 쓰임에 대해 남겨주실 수 있을까요?
There was a problem hiding this comment.
지금 상태로는 단순히 List만 들고 있어 구조적으로 다소 허전하게 느껴질 수 있다는 점에 깊이 공감했습니다.
그래서 이 클래스는 getConnections()처럼 내부 상태를 그대로 노출하기보다는,
→ drawLineFormat()과 같은 의미 중심의 메서드로 표현 책임을 도메인 내부로 옮겨보겠습니다.
사실 처음에도 표현 책임을 도메인으로 넘길지 고민했었는데요,
그 당시에는 “이건 View의 역할에 가까운 게 아닐까?”라는 생각이 들어서 머뭇거렸던 것 같아요.
하지만 다시 생각해보니, 도메인 개념인 사다리 줄(BridgeLine)이 스스로 자신의 상태를 그릴 수 있어야 하지 않을까 싶기도 하고,
오히려 Board, PlayerResults에 비해 이 클래스의 역할이 부족하다는 느낌도 있었던 터라,
이번 기회에 이 부분을 좀 더 책임 있게 리팩토링해보겠습니다! 🙌
There was a problem hiding this comment.
아 근데 사실 제가 이 리뷰 당시에 요구사항을 정확하게 파악하지 못했던 것도 있기는 해서, 이대로의 구현도 나쁘지는 않았을 것 같긴 해서 머쓱하네요 ㅎㅎ..
| import java.util.Arrays; | ||
| import java.util.List; | ||
|
|
||
| public class ValidatedInputParser { |
There was a problem hiding this comment.
클래스의 역할이 좀 애매한 것 같아요. 이 클래스가 의도하는 책임은 무엇일까요? 다음 중 어떤 책임이 올바르다고 생각하나요?
- String으로 나온 rawData를 형변환 하거나 split 하는 등 알맞는 타입으로 변경해주는 것
- 도메인 규칙을 검증하는 것
There was a problem hiding this comment.
음... 제 생각에는 사실 두 가지 책임 모두 ValidatedInputParser가 가져야 할 책임은 아닌 것 같아요!
-
도메인 규칙 검증은 말씀해주신 대로 도메인 내부에서 담당해야 자연스럽고 견고하다고 생각하고,
-
형변환이나 파싱(splitting)은 View 쪽에서 입력 처리의 연장선으로 다루는 게 더 어울린다고 느껴졌어요.
그래서 이 클래스는 지금 구조상 책임이 애매하게 중간에 걸쳐 있는 상태라고 생각했고,
오히려 명확한 역할 분리를 위해선 제거하거나, 책임을 명확히 위임하는 방향으로 개선하는 게 더 나을 것 같다는 생각이 들어요!
There was a problem hiding this comment.
맞아요. 형변환(그 과정에서 타입이 맞는지 검증), 파싱은 뷰에서 처리하고(뷰가 정말 커지면 로직만 Parser를 만들어서 옮길 수는 있겠죠), 도메인 규칙 검증은 도메인 내부로 이동하도록 의도한 리뷰였습니다!
src/main/java/domain/LadderPath.java
Outdated
| private int moveAlongLine(int position, BridgeLine line) { | ||
| if (position > 0 && line.isConnectedAt(position - 1)) return position - 1; | ||
| if (position < line.width() && line.isConnectedAt(position)) return position + 1; | ||
| return position; | ||
| } |
There was a problem hiding this comment.
사실 처음엔 LadderPath의 역할이 너무 줄어드는 게 아닐까? 하는 고민도 조금 있었던 것 같아요.
괜히 너무 비워지는 건 아닐까 싶은 마음이었는데요 😅
하지만 말씀해주신 것처럼, 어떤 위치에서 다음 위치로 이동할지를 판단하는 책임은
그 줄의 연결 상태를 가장 잘 알고 있는 BridgeLine이 맡는 게 훨씬 자연스럽고 응집도 있는 구조라는 생각이 들었습니다.
그래서 해당 로직을 BridgeLine으로 위임하고,
LadderPath는 순회 흐름만 깔끔하게 담당하는 역할로 정리해 보겠습니다!
There was a problem hiding this comment.
사실 흐름 제어도 객체의 중요한 역할이기 때문에! LadderPath의 역할이 너무 적은 것 아닐까? 라고 걱정할 필요는 없을 것 같아요!
src/main/java/domain/LadderPath.java
Outdated
| int current = startColumnIndex; | ||
|
|
||
| for (BridgeLine line : bridgeLines) { | ||
| current = moveAlongLine(current, line); | ||
| } | ||
|
|
||
| return current; |
There was a problem hiding this comment.
Line을 따라 계속 움직이는거죠? 그럼 변수명이 current면 안될 것 같아요!
There was a problem hiding this comment.
맞아요! 말씀해주신 걸 보고 나서야 아차 싶었어요 😅
이 로직은 가로줄(BridgeLine)을 따라 계속 이동하는 흐름을 표현하는 건데,
변수명이 current라고 되어 있으니 위치 값을 의미하는 건지, 상태를 의미하는 건지 모호하게 느껴질 수 있겠다는 생각이 들었습니다.
말씀해주신 대로 좀 더 의도를 드러내는 네이밍으로 개선해보겠습니다!
| @ParameterizedTest(name = "index={0}일 때 연결 여부는 {1}이다") | ||
| @MethodSource("connectionTestCases") | ||
| @DisplayName("isConnectedAt는 주어진 인덱스의 연결 상태를 반환한다") | ||
| void isConnectedAt_returnsCorrectStatus(int index, boolean expected) { | ||
| BridgeLine line = new BridgeLine(List.of(false, true, false)); | ||
| assertThat(line.isConnectedAt(index)).isEqualTo(expected); | ||
| } |
There was a problem hiding this comment.
이 테스트는 given이 같은데 @ParameterizedTest를 사용할 필요가 없을 것 같아요
There was a problem hiding this comment.
여러 가지 값을 테스트할 땐 자연스럽게 "아, 파라미터 테스트 써야겠다!"는 생각이 먼저 들었던 것 같아요!
그런데 오찌님 말씀 듣고 보니 정말 그렇네요 😅
이번 테스트는 BridgeLine 인스턴스를 하나만 만들어놓고,
그 내부 상태를 다양한 인덱스로 확인하는 구조라서 given이 고정된 상황이더라고요.
그래서 굳이 @ParameterizedTest까지 사용할 필요는 없었던 것 같아요!
오히려 각 인덱스별로 의도를 명확히 드러내는 단위 테스트로 나누는 편이,
-
테스트 이름만 보고 어떤 조건을 검증하는지 바로 파악할 수 있고
-
실패했을 때도 어떤 케이스에서 실패했는지 명확해서 디버깅도 훨씬 쉬울 것 같다는 생각이 들었습니다.
그래서 이 부분은 @ParameterizedTest 대신 명시적인 테스트 메서드들로 분리해서 개선해보겠습니다!
예리한 피드백 덕분에 한 번 더 고민해볼 수 있었어요. 감사합니다 !!
| private static boolean isConnectedAtCenter(BridgeLine line, int centerIndex) { | ||
| return centerIndex < line.width() && line.isConnectedAt(centerIndex); | ||
| } |
There was a problem hiding this comment.
그런데 isConnectedAtCenter는 어떤 의도로 필요한가요? 프로그램 요구사항 중에 중앙 사다리에 반드시 연결되어 있어야 한다는 못본 것 같아서요!
There was a problem hiding this comment.
아, 이 부분은 구현하면서 중간에 추가한 로직인데요!
중간에 사다리가 너무 한쪽으로만 쏠리거나, 중앙이 전혀 연결되지 않아서
사다리 전체가 갈 길을 잃은 듯한 구조가 만들어지는 걸 실제로 여러 번 봤거든요 😅
그래서 "최소한 중앙은 한 번쯤 연결되어 있어야 흐름이 생기겠다"는 생각으로
isConnectedAtCenter 조건을 넣게 되었습니다.
물론 말씀해주신 것처럼, 요구사항 상에 ‘중앙 연결이 필수’라는 조건은 명시되어 있지 않아요.
그럼에도 이 조건을 추가한 이유는, 랜덤으로 생성되는 특성상
흐름이 단절되거나 사다리가 지나치게 한쪽으로 치우치는 걸 방지하고 싶었기 때문이에요.
다만 지금 다시 보니, 요구사항에 없는 조건을 도메인 로직처럼 넣어버린 게
다이어트를 하다가 정크푸드(객체지향)를 먹은 느낌이네요! 😅 개선하는 게 좋을까요!?
There was a problem hiding this comment.
아하 그런 의도에서였군요. 그런 의도라면 굳이 제거할 필요는 없을 것 같습니다! 다만 제가 테스트해보지는 않은건데, 정말 많은 수의 인원이 참여하는 게임에서도 잘 연결되는지 체크해보면 더 좋을 것 같아요!
|
아 추가로 코멘트 남기자면 OutputView가 결과물을 들고가서 알아서 출력하는 것은 이상한 것이 아니라고 생각해요~! |
피드백 남겨주셔서 감사합니다! 🙇♀️ 말씀해주신 내용들을 읽으면서, 제가 View를 '날씬하게' 유지해야 한다고 느꼈던 이유가 어디서 왔는지 다시 돌아보게 되었어요. 저는 View는 비즈니스 로직과 철저히 분리하는 것이 유지보수 측면에서 더 유리하다고 생각해왔습니다. 그래서 View가 너무 많은 책임을 가지기 시작하면 자연스럽게 도메인 로직과 엮이게 되고, 이로 인해 UI 변화가 비즈니스 로직에 영향을 줄 수 있겠다는 우려가 있었어요. 그런 관점에서 View는 최대한 '얇고 단순한 역할'을 수행해야 한다는 생각이 있었던 것 같아요. 하지만 말씀해주신 것처럼, View가 입출력을 담당하는 클래스라면 입출력과 관련된 책임이 커지는 것도 자연스러운 흐름이라는 의견에 깊이 공감하게 되었습니다. 특히 "파싱"이라는 행위도 입출력의 연장선에 있다고 본다면, 그것을 View에서 다루는 것이 이상한 일만은 아니라는 걸 새삼 느꼈어요. 저도 이번 기회에 'View가 정말 어디까지 책임져야 하는가'에 대해 다시 한번 고민해보게 되었고, 기존에 갖고 있던 고정관념을 유연하게 바라보게 된 것 같아요. 좋은 질문과 생각할 거리 던져주셔서 진심으로 감사드립니다! 🌱 그리고 말씀 주신 5단계 함수형 프로그래밍 적용도 꼭 반영하겠습니다! 놓치지 말아야 할 부분이었는데, 단순한 리팩터링이라고 생각해서 미처 신경 쓰지 못했네요 😅 이번 리뷰 주신 부분은 말씀 주신 방향에 맞춰 수정하고, 빠르게 다음 단계로 진행해보겠습니다! 꼼꼼한 피드백 덕분에 하나씩 놓쳤던 부분을 바로잡을 수 있어 늘 감사한 마음입니다 🙏💛 |
|
Step5 리팩터링(함수형 프로그래밍 적용) 반영해보았습니다! 😊 가능한 많은 부분에 함수형을 적용하려고 했지만, 모든 곳에 무조건적으로 적용하는 건 오히려 가독성이나 성능 면에서 손해일 수 있다고 판단했습니다! 그래서 이번에는 함수형으로 전환했을 때 코드의 의도가 더 잘 드러나거나 성능 이점이 있는 부분 위주로 선택적으로 리팩터링을 진행해보았습니다! 읽어보시고 부족한 부분이나 개선할 점 있다면 편하게 피드백 부탁드립니다! 🙏 |
| private List<String> promptParticipants() { | ||
| outputView.printParticipantPrompt(); | ||
| return inputView.readParticipantNames(); | ||
| } |
There was a problem hiding this comment.
view의 메서드 이름이 printXXXPrompt 여서, promptXXX 메서드의 prompt가 동사로 의미가 와닿지 않는 것 같아요. read나 input이어도 충분하지 않을까 싶습니다!
There was a problem hiding this comment.
메서드 네이밍이 확실히 직관적으로 와닿지 않는다는 점, 말씀 듣고 보니 정말 공감됩니다!
당시에 인덴트를 1단계로 유지하면서, 컨트롤러 내부 로직을 private 메서드로 분리하는 데 집중하다 보니,정작 메서드 이름 자체가 전달하는 의미에 대해서는 깊이 고민하지 못했던 것 같아요 😅
각 View의 메서드를 호출할 때는 의미를 고려해서 네이밍했는데, 그걸 조합해서 컨트롤러 메서드를 만들 때는 의미보다 구조에만 집중한 흔적이 남아 있었네요.
이 부분은 네이밍을 조금 더 고민해서 개선해보겠습니다!
피드백 감사합니다! ✨
| if (handleSingleResult(playerResults, name)) continue; | ||
| if (handleAllResults(playerResults, name)) return; |
There was a problem hiding this comment.
while 안의 if는 continue, return을 같은 줄에 쓴다고 indent가 1인 것일까요? indent를 1만 허용하는 규칙의 의도는 그게 아닐 것 같아요!
There was a problem hiding this comment.
네, 정말 공감되는 부분입니다…! 😅
사실 이 부분은 인덴트를 억지로 1로 맞추기 위해 continue, return을 바로 붙여 처리했는데요,
말씀해주신 대로 “인덴트 1만 허용”이라는 규칙의 본질은 단순히 줄 수를 줄이는 게 아니라,
명확하게 책임을 분리하라는 의도였다는 점을 다시금 깨닫게 되었습니다.
특히 while이 들어가는 순간 인덴트를 1로 유지하는 방법에 대한 고민이 많았어요.
예외 메시지를 출력하고 재입력을 받는 구조를 유틸 클래스로 분리해도
내부에서 다시 분기가 발생하게 되니 인덴트 제한을 완전히 지키기 어렵더라고요 😥
함수형 스타일로 풀어보려고 해도 결국 조건문이 중첩되어버리는 상황이 반복됐습니다.
그래서 일단은 인덴트 1을 유지하기 위해 while 자체를 제거하는 방향으로 리팩터링을 시도해보겠습니다! while을 유지하면서도 인덴트를 1로 유지할 수 있는 좋은 구조나 방법이 있다면
정말 배워보고 싶습니다..! 🙏
src/main/java/domain/BridgeLine.java
Outdated
| public String drawLineFormat(int columnCount) { | ||
| return IntStream.range(0, columnCount) | ||
| .mapToObj(i -> VERTICAL_BAR + renderBridge(i)) | ||
| .collect(Collectors.joining()); | ||
| } | ||
|
|
||
| private String renderBridge(int index) { | ||
| if (index >= horizontalConnections.size()) { | ||
| return EMPTY_LINE; | ||
| } | ||
|
|
||
| if (horizontalConnections.get(index)) { | ||
| return CONNECTED_LINE; | ||
| } | ||
|
|
||
| return EMPTY_LINE; | ||
| } |
There was a problem hiding this comment.
선을 그리는 것은 뷰의 책임이 아닐까요? 도메인에서는 선 모양을 그려주는 것이 아니라 어떻게 선이 있음/없음을 나타낼 수 있을까를 고민하는 것이 더 좋을 것 같아요. 이 프로그램이 콘솔로 입출력을 하는게 아니라 프론트엔드에 그림을 그려주는 프로그램으로 바뀐다면, 지금처럼 콘솔에 최적화된 drawLine이 있다면 변경이 도메인까지 전파되지 않을까요?
There was a problem hiding this comment.
단순히 Line이 하는 일이 적어 보이다 보니, 뷰의 역할 일부를 도메인 쪽에 넘겨버린 것 같아요 😅
당시에는 출력을 도메인 내부에서 처리하는 게 간편하다고 생각했는데,
말씀해주신 것처럼 콘솔 기반 출력에 최적화된 로직이 도메인에 들어간 상태라
향후 출력 방식이 바뀔 경우 도메인까지 변경 영향을 받게 될 수 있다는 점, 정말 공감됩니다.
사실 BridgeLine이 “선을 어떻게 그릴까”가 아니라,
“이 위치에 연결이 있는가?”라는 메시지를 중심으로 협력할 수 있게 설계했어야 했는데,
그 부분을 깊이 고민하지 못했던 것 같아요.
말씀 주신 방향대로, 도메인은 표현이 아닌 상태와 행위만 담당하고,
출력 형식은 View 계층에서 해석할 수 있도록 구조를 개선해보겠습니다.
좋은 피드백 감사합니다! 🙏✨
| if (!areAllColumnsConnected(connectedColumnIndices, participantCount - 1)) return null; | ||
| if (!isCenterBridgePresent) return null; |
There was a problem hiding this comment.
메서드 시그니처만 보고는 null 반환을 기대하기는 어려울 것 같아요!
또한 일반적으로 null 반환은 그닥 권장드리지 않습니다. 사용하는 쪽에서 메서드 시그니처만 보고 null 방어 코딩을 하는 것이 어렵기도 하고, 이에 따라 NPE의 발생 가능성이 올라가거든요. 정말 필요한 상황에서는 사용해야겠지만, 저는 이런 상황이라면 메서드 명에 orNull 등을 명시하는 편입니다.
null을 사용한다면 차라리 Optional을 사용해서 최소한 사용하는 측에서 null 방어를 할 수 있도록 하는 것이 적절하고, 아니면 예외를 통한 제어를 많이 사용하는 것이 좋을 것 같아요.
There was a problem hiding this comment.
피드백 감사합니다! 🙏
말씀해주신 것처럼, 메서드 시그니처만 봤을 때 null이 반환될 거라고 기대하긴 어렵겠네요!
이 로직에서는 랜덤하게 생성된 사다리가 유효 조건을 만족하지 못했을 때 실패를 나타내기 위해
간단하게 null을 반환했는데요, 지금 와서 보니 구조적으로도 그렇고 시맨틱하게도 아쉬운 점이 많았던 것 같습니다 😅
말씀해주신 대로 아래 방향 중 하나로 개선해보는 쪽이 더 적절할 것 같아요:
- Optional<List>로 감싸서 호출하는 쪽에서 의도를 명확하게 인식하고 방어하도록 유도
- 혹은 조건 실패 시 IllegalStateException 등의 예외를 던져 실패를 명시적으로 드러내는 방식
개인적으로는 사다리 생성이 실패할 수 있는 "예외적인 상황"으로 보기 때문에
예외를 사용하는 방향으로 먼저 적용해보고, 이후 필요하다면 선택적으로 Optional 도입도 고려해보겠습니다!
|
|
||
| public Map<Integer, Integer> mapStartToEndIndex() { | ||
| return IntStream.range(0, numberOfColumns) | ||
| .boxed() |
There was a problem hiding this comment.
피드백 감사합니다! 🙇♂️
말씀 주신 내용을 보고 "혹시 정말 불필요한 오토박싱이 있었던 건 아닐까?" 하고 저도 다시 한 번 살펴봤는데요, 이번 경우에는 boxed()가 기능적으로 필수적이라고 느껴지더라구요!
현재 mapStartToEndIndex()에서는 IntStream.range(...)을 사용한 후
.collect(Supplier, BiConsumer, BiConsumer)를 통해 Map<Integer, Integer>로 변환하고 있는데요,
이 collect() 메서드는 프리미티브 stream(IntStream)에서는 직접 사용할 수 없고,
Stream로 박싱된 스트림이어야만 사용 가능한 형태더라고요.
즉, .boxed()를 제거하면 collect(...) 자체가 불가능해서 컴파일 에러가 발생합니다 😅
물론 말씀하신 대로 boxed()를 사용하지 않으려면 전통적인 for문을 사용하는 방식으로 바꾸면 되긴 하지만, 이번 구현에서는 함수형 프로그래밍 방식으로 리팩토링하는 목적도 있었기 때문에 스트림을 유지하려면 boxed() 사용이 불가피한 상황이었습니다!
혹시 제가 이 부분에서 잘못 이해하고 있는 점이 있다면 피드백 주시면 정말 감사하겠습니다 🙏
오찌 다시 한 번 스트림의 작동 방식과 선택의 이유를 정리해볼 수 있었어요! 😊
There was a problem hiding this comment.
흠, 제가 boxed 제거해보니까 잘 되는데요! Collectors.collect 를 사용할 때는 boxed가 필수적인데, collect(Supplier, BiConsumer, BiConsumer) 메서드 사용에는 boxing이 필요한 것 같지는 않아보여요! 다시 한 번 체크해보시겠어요?
There was a problem hiding this comment.
말씀해주신 것처럼, 이번 경우엔 boxed() 없이도 collect가 동작하네요!
초기에 제가 Collectors.toMap과 혼동해서 생긴 착각이었던 것 같습니다 😅
피드백 감사드립니다! 🙇♂️🙏
| validateNotEmpty(playerNames); | ||
| validateNameLength(playerNames); | ||
| validateSizeMatch(playerNames, outcomeLabels); |
There was a problem hiding this comment.
이런 검증은 결과 생성이 아니라 사다리 생성 전에 검증이 되어야 할 것 같아요! 필요하다면 Participants 같은 클래스 분리도 적절히 활용하면 좋을 것 같습니다!
There was a problem hiding this comment.
피드백 감사합니다! 🙇♂️
말씀해주신 내용을 보고 저도 다시 보니 정말 그렇다는 생각이 들었습니다.
지금 구조에서는 PlayerResults가 결과 매핑의 역할뿐만 아니라
입력값의 유효성 검증까지 함께 책임지고 있어서 응집도가 다소 흐려져 있는 상태인 것 같아요.
사다리를 생성한 이후에 결과를 매핑하는 시점에서
참여자 이름이 비어 있거나, 길이가 초과되었는지를 검증하는 건 시점상 어색할 뿐 아니라,
책임 위치도 명확하지 않다는 점에 깊이 공감했습니다.
사실 처음 이 구조를 설계할 때도 이름이나 사람을 클래스로 분리해볼까 하는 고민이 있었는데,
괜히 지나치게 쪼개는 게 아닐까 싶어 망설였던 기억이 있어요.
하지만 본문 피드백에서 말씀해주신 것처럼, 적절한 책임 분리는 필요하고,
그걸 실제로 분리해볼 수 있는 역량도 중요하다는 말씀이 크게 와닿았습니다.
그래서 이번 기회에 Participants와 같은 도메인 객체를 도입해서
유효성 검증 책임을 이전 단계에서 처리하는 구조로 리팩토링해보려고 합니다.
예를 들면 Participants.of(names) 내부에서
-
이름 리스트가 비어 있는지
-
각 이름의 길이가 적절한지 등의 검증을 수행하고, PlayerResults는 그 이후의 결과 매핑 역할에만 집중할 수 있도록 구조를 정리할 계획입니다.
이번 피드백 덕분에 단순히 검증을 수행하는 것에 그치지 않고,
“검증이 언제, 어디서 이뤄지는 게 가장 자연스러운가”에 대해서도 다시 한 번 고민해볼 수 있었습니다.
좋은 피드백 주셔서 감사합니다! 🙏✨
| @@ -0,0 +1,14 @@ | |||
| package util; | |||
|
|
|||
| public class NumericParser { | |||
There was a problem hiding this comment.
아이코,, 말씀 듣고 보니 정말 그렇네요! 😅
원래는 InputView 쪽에 붙여서 입력값을 파싱하는 책임을 함께 갖도록 할 계획이었는데,
리팩터링을 하면서 따로 분리해놓고는 실제로 활용하지 못하고 있었던 것 같아요.
테스트 용도로만 쓰이고 있다면 굳이 따로 클래스로 뺄 필요는 없겠다는 생각이 들어서,
InputView에 다시 통합하거나, 혹은 필요하다면 명확한 책임을 부여한 형태로 다시 활용할 수 있도록 정리해보겠습니다!
날카로운 피드백 정말 감사드립니다 🙏
리팩터링 과정에서 무심코 놓치게 되는 부분들이 있다는 걸 다시금 느꼈고,
앞으로는 이런 부분들까지 더 세심하게 살펴봐야겠다는 생각이 들었습니다!
| private boolean isAllCommandAndNotPlayer(String name, PlayerResults playerResults) { | ||
| return "all".equals(name) && !playerResults.hasPlayer(name); | ||
| } |
There was a problem hiding this comment.
개인적으로는 그냥 all 같은 예약어는 이름으로 못받게 하는게 좋을 것 같아요. 만약에 사용자 이름에 all이 있으면 전체 결과 출력을 못받을 것 같네요.
There was a problem hiding this comment.
피드백 감사합니다! 🙏
말씀 듣고 보니 정말 그렇네요! 생각해보면 all처럼 특정 키워드를 사용자 입력으로 받는 걸 막지 않으면, 의도치 않게 전체 결과를 요청하려던 동작이 사용자 이름 때문에 막히는 문제가 생길 수 있겠어요.
사실 지금까지는 "형식이 틀렸으면 예외를 던진다"는 흐름에 익숙하다 보니,
"특정 값 자체를 거부한다"는 방식은 조금 낯설게 느껴졌던 것 같아요 😅
이번 기회를 통해 입력값 자체에 예약어 제약을 두는 것도 중요한 방어 로직이라는 걸 배운 느낌입니다!
말씀해주신 방향대로 all은 이름으로 받을 수 없도록 제한을 걸어보겠습니다 :)
3 participants
사다리 미션 회고
안녕하세요 오찌! 🙌
이번에는 로또 미션의 피드백을 바탕으로 다음 부분을 위주로 노력하였어요!
위와 같은 목표를 세우고 미션을 구현해보니 새삼 놀라웠습니다. 이렇게 작은 클래스들로 구현이 가능했던가... 싶었어요. 물론 로또보다 조금 덜 복잡한 요구사항 같긴 합니다. 그럼에도 불구하고(1) 기존의 하던 습관대로 분리를 진행했으면 훨씬 복잡하게 구현했을 것 같다는 생각이 들어 이전 피드백의 도움을 많이 받았다는 생각이 들었습니다. 감사합니다! 😊
그럼에도 불구하고 (2) 고민은 있었는데요! 🤔
1.
LadderBoard괜찮은가?2. 검증, view 너무나도 분리하고 싶었다.
요구사항을 보면 잘못된 입력값에 대한 처리 부분이 없었습니다. 그래서 검증을 해야 하나 말아야 하나 고민이 들었고, 최소한의 검증만 진행하기로 판단했습니다.
그러자 이제 파싱과 검증을 어떻게 해야 할 것인가 고민이 들었습니다. 파싱 나누고, 검증 나누면 요구사항에 비해서 또 복잡도가 커질 것 같아서 최대한 간결하게 입력과 검증을 하나로 묶었습니다.
기존에 있던 요구사항도 아니었기에 최대한 가볍게 거치는 정도로 구성해야겠다고 생각했습니다. (기존 같았으면 DTO, View, Mapper, Parser, Validate, VO 나눴을 것 같습니다.) 이렇게 구성하니 읽기도 좋고 간편함이 느껴지기도 합니다.
3. 출력과 비즈니스 로직의 경계
3, 4단계부터 결과에 대한 요구사항이 추가되었는데 핵심 비즈니스 로직은
BridgeLine,LadderPath,LadderBoard에 담겨 있었고결과들은 요구사항에 맞게 변환만 해주는 것이었는데요, 그래서 이 로직을 View에 담아야 하는 거 아닌가라는 생각이 계속 들었습니다.
그래서 이 부분에 대해서 각각의 결과 ( 사다리 결과(1), 결과를 보고 싶은 사람에 대한 결과(2), 전체 실행 결과(3) ) 이 부분을 어떻게 처리해야 할까 생각을 하다, 최대한 핵심들만
PlayerResults에 담아내고OutputView로 몰아냈습니다.그래서 기존에 제가 작성하던 코드들에 비해서
OutputView에 로직이 담겨 있다고 느껴졌는데요.OutputView구성에 대해서 어떻게 생각하시는지 궁금합니다!4.
InputView의 가벼움사실 이번 미션이 depth는 가져가고 옆으로는 날씬하게 구현을 하려고 했지만
InputView를 보면 너무 다이어트를 한 게 아닌가 싶기도 한 생각이 들었습니다.그래서 중간에 파싱, 검증 부분에 있는 로직을
InputView에 붙일까 했는데… 날씬하다는 이유만으로 나누기에는 근거가 부족한 것 같아서 이렇게 구성했는데 이러한 상황에 대해서 어떻게 생각하시는지 궁금합니다!5. 클래스 다이어트
이번 미션에서는 의식적으로 근거 있게 구성을 하려고 했고 테스트 코드도 꼭 필요하다고 생각하는 부분들을 위주로 구성했는데요.
전반적으로 너무 과하게 응집되었는지, 아니면 분리를 하면 좋았겠다 싶은 부분 등 이번 미션에도 역시 가감 없이 피드백 주시면 감사하겠습니다!