[2단계 - 사다리 게임 실행] 져니(이지원) 미션 제출합니다. #156
Conversation
- 컨트롤러에서 LadderFactory 대신 Ladder를 호출하도록 수정하였으며, LadderFactory의 생성 접근 제어자를 protected로 변경하였습니다. - 또한, 이에 따라 테스트 코드를 LadderFactory -> Ladder로 이동시켰습니다.
- ladderHeight에 대해 전역적으로 쓰이지 않고 있기 때문에 init 대신에 그냥 메서드 내부에서 선언하도록 변경하였습니다. - 변수명을 ladderResultOrder -> ladderResultPositions로 변경하였습니다.
- protected보다 default가 더 엄격한 제어자이기 때문에, 변경하였습니다. (상속이 불가능하도록)
- 기존 사다리 로직 메서드에서 조금 더 간결해지도록 수정하였습니다. - 기존은 add를 통해서 가장 첫 패딩이 들어간 사다리 출력부분을 붙여주고, 그 뒤로도 계속 add를 통해 붙이는 로직이었는데, StringJoiner를 제거하고 collect 시 prefix 부분에 첫 패딩 부분도 함께 추가되도록 하여 로직을 개선하였습니다.
- all을 입력받으면 전체 참여자 이름 리스트를 반환하도록 하였습니다.
- Ladder의 LadderRung의 필드를 생성해주는 역할에 더 가깝다고 생각이 되어서 네이밍을 변경해주었습니다.
- 참여자 이름에서 참여자 자체를 반환하도록 수정하였습니다.
- 사다리 게임 진행 시 참여자 수는 외부에서 제어할 항목이고, 사다리 게임을 진행할 때 알 필요 없는 요소라고 생각이 되어서 컨트롤러에서 참여자의 순서만 전달해주도록 수정하였습니다.
- 컨트롤러에서 게임 실행 후 최종 위치를 받아서 넘겨주는 형식으로 진행했습니다!
There was a problem hiding this comment.
안녕하세요 져니!
2단계 금방 진행해주셨네요!! 개인 일정으로 늦은 리뷰 양해 부탁드렸는데 양해해주셔서 감사합니다 🙇🏻
전체적으로 읽기 좋은 코드였어요 ㅎㅎ
질답시간
커밋 단위
시작부터 완벽한 설계를 알고 있다면 리팩토링을 첫번째 커밋에서 끝낼 수 있겠지만 잘 안 되는 경우가 많다고 생각해요 ㅎㅎ 짜다보면 어질어질해지고 리팩토링 해야겠다 싶을 때가 오는 때가 더 많은 것 같네요. test와 feat 정도는 하나의 커밋에서 다룰 수 있다고 생각하구요, 그 이후 추가적으로 리팩토링 한 것을 한 커밋에 담으려고 노력하지 않으셔도 될 것 같습니다. (중요한건 해당 커밋이 제대로 동작하는가 같네요) 더군다나 저희는 피쳐 브랜치를 squash merge하는 상황인 만큼, '너무 커밋을 잘게 쪼개나' 하는 고민이 작아지기두 하고요.
도메인의 예외 메시지와 테스트 코드
ㅎㅎ 클래스가 너무 많아지면 안 되나요? 에러메시지를 미리 쪼개놓거나 범용 예외 객체에서 느껴지는 감상인데 져니는 범용적인 객체를 만들어 코드 중복을 줄이는 것에 가치를 두신 것 같아요. 코드 중복을 줄이기 위해 노력하는 건 좋은 자세이므로 아래에 쓰는 건 공감이 된다 싶으면 들어주시고 이해가 안 되거나 공감이 안 되면 패스해주세요 ㅎㅎ
저는 여기저기서 다 쓸 수 있는 객체를 좋아하지 않습니다. 확장성 이란 말은 "이 객체가 현재 쓰임 말고도 다른 곳에서도 쓰일 수 있게" 라는 말로 들리지만, 사실 정반대라고 생각합니다. 특정한 객체는 "현재 쓰임"에만 집중하고 역할을 다할 수 있게 해야하고, 다른 객체를 의존할 일이 있다면 그 객체가 "특정 역할"만 수행하는지만 신경쓰는 게 확장성이라고 봐요. 여기저기서 다 쓰이는 객체는 여러 객체들과 연관관계를 맺게 되고, (다른 말로 결합도가 높아짐) 점점 해당 객체를 수정하기 어렵게 만듭니다. 블라블라..
여하튼 그래서 일단 현재 쓰임에 가장 적합하게 객체를 만들어주시고 나중에 공통화 할 수 있는 기능(다른말로 추상화)이 보일 때 추출해보는 연습을 해보시는 것도 좋을 것 같아요. 지금도 충분히 잘하시지만요!!
LadderFactory와 Ladder
디자인패턴의 장점 중에는 "여러 개발자의 멘탈모델을 아주 쉽게 맞출 수 있다"는 점이 있는데요, 그런 점에서 객체 명에 특정한 디자인 패턴을 연상시키는 이름을 적어주면 그걸 본 개발자는 뭔가 떠올리게 됩니다. Factory라는 케이스에선 객체 생성자를 떠올리게 될텐데요, Rungs에서 복수형으로 붙여준 s는 대게 컬렉션의 표현이기도 하니, 위 두 개의 정보를 종합했을 때 create()에서 List<Rung> 을 반환하는 걸 보면 아항 Rung의 컬렉션을 반환하겠구나.. 이해하지 않을까요? "팩토리인데 왜 컬렉션이 나와?" 라고 주장하시는 개발자도 계실 수 있겠지만.. 디자인 패턴의 장점을 극대화하기 위해 엄격하게 해당 패턴을 지켜야 한다고 주장하신다면.. 전 그냥 "넵 그럼 LadderRungsCreator로 수정하겠습니다 ㅎㅎ" 라고 할 거 같긴하네요
접근제어자를 더 좁혀주신건 좋습니당 ㅎㅎ 더 나아가시는 모습 멋지네요
사다리 게임 실행 로직에 대한 고찰
ㅋㅋ 절 러버덕으로 쓰셨군요 좋습니다
컨트롤러 길이 같은 경우 엄격하게는 100줄 넘으면 분리를 고려하라는 말도 있는데요, 저같은 경우 public api에만 대입을 해보는 편이에요. private은 길어져도 크게 신경쓰지 않습니다 (개인 의견)
실행 종료에 대한 로직
이건 리뷰로 드렸습니다 ㅎㅎ
전반적으로 잘 해주셔서 또 머지욕구가 올라왔는데요,
크게 실행 종료에 관한 로직과 결과 숫자가 왼쪽으로 쏠리는 부분만 해결해주셔요 ㅎㅎ
기타 리뷰 반영은 자유롭게 해주십숑
| @@ -0,0 +1,55 @@ | |||
| package laddergame.domain.ladder_result; | |||
There was a problem hiding this comment.
ladder_result 대신 ladder.result 도 가능하지 않았을까요? 언더바로 이은 선택의 이유를 들을 수 있을까요? ㅎㅎ
There was a problem hiding this comment.
아… 이거는 제가 생각을 못했던 부분이었어요. ㅋㅋㅋㅋㅋ LadderResult에 대한 패키지 이름은 어떻게 하지? 패키지 이름은 소문자여야 하는데…? 하다가 언더바(_)를 사용하면 된다는 글을 보고 적용했는데, 그냥 Ladder의 내부로 넣는 방식도 있겠네요. 바로 수정해두었습니다 ㅎㅎ 👍
|
|
||
| private void validateNameSize(final List<LadderResultName> ladderResultNames, final int participantCount) { | ||
| if (ladderResultNames.size() != participantCount) { | ||
| throw new IllegalArgumentException(String.format(INVALID_LADDER_RESULT_SIZE, LADDER_RESULT_SIZE, participantCount)); |
There was a problem hiding this comment.
INVALID_LADDER_RESULT_SIZE는 한 곳에서만 쓰이는데 LADDER_RESULT_SIZE와 합쳐줘도 되지 않을까요?
There was a problem hiding this comment.
아 그러네요! 아마 예외 메시지에 대해 계속 바꿔가면서 놓친 것 같아요. 수정해두겠습니다 👍
| do { | ||
| resultParticipants = getResultParticipants(participants); | ||
| printGameResult(resultParticipants, ladderResultNames); | ||
| } while (isProceed(resultParticipants)); |
There was a problem hiding this comment.
질문도 주셨지만 눈에 띄네요 ㅎㅎ
말씀하신것처럼 null 반환은 쉬운 해결책임과 동시에 가시밭길입니다. NPE는 코드에게 꿀밤을 때리고 싶다는 감정을 일으키는 주범이거든요 한 번 null을 쓰면 모든 곳에서 null check를 해주어야 합니다.
그렇다고 예외를 비즈니스 로직에 활용하겠다는 건 좋은 계획은 아닙니다. 특히나 unchecked 예외를 활용하겠다는 계획이 위험한데, 기존에 던져지던 unchecked 예외가 다른 예외로 바뀌어도 컴파일러가 감지를 하지 못하기 때문입니다. 그렇다고 Exception을 catch하면 온갖 원인으로 인해 시스템이 멈출 거구요.
어떻게 해볼까요? isProceed가 null을 받아야 했던 이유가 뭘까요? List Type이 "실패했다"는 개념을 표현해주지 못 하기 때문 아닌가요? 해당 타입이 이 역할을 맡기에 적합하지 않네요. List도 가지고 있고, 실패도 표현할 수 있는 객체를 만들어주어야 겠습니다 ㅎㅎ
| if (resultParticipants == null) { | ||
| return; | ||
| } | ||
| OutputView.print(System.lineSeparator() + RESULT_GAME_GUIDE.getMessage()); |
There was a problem hiding this comment.
요 부분도 outputView 객체가 처리해도 되지 않을까요? ㅎㅎ
| @@ -1,18 +1,43 @@ | |||
| package laddergame.domain.participant; | |||
|
|
|||
| import java.util.Objects; | |||
|
|
|||
| public class Participant { | |||
|
|
|||
| private final ParticipantName participantName; | |||
There was a problem hiding this comment.
변수명은 name과 order로 축약해도 되지 않을까 싶습니다 ㅎㅎ
| private void validateParticipantCount(final List<String> participantNames) { | ||
| if (participantNames.size() == MIN_COUNT) { | ||
| throw new IllegalArgumentException(INVALID_PARTICIPANT_COUNT); | ||
| throw new IllegalArgumentException(String.format(INVALID_PARTICIPANT_COUNT, PARTICIPANTS, MIN_COUNT)); |
There was a problem hiding this comment.
요기두 INVALID_PARTICIPANT_COUNT가 한 군데에서만 쓰이는데 PARTICIPANTS랑 메시지를 합쳐줘도 괜찮지 않을까요 ㅎㅎ
|
|
||
| ## ✔️ 기능 요구사항 | ||
|
|
|
아침에 다시 읽어보니 리뷰 코멘트에 오해의 소지가 있는 부분들이 있는거 같아 정리하겠습니다 ㅎㅎ
수정하라는 건 아니었고 저는 네이밍이 상대의 멘탈모델을 방해한다면 수정할 수 도 있다는 이야기였습니다 ㅎ_ㅎ
위 부분은 컨트롤러 뿐만 아니라 모든 코드를 염두하고 말하였습니당 |
- ladder_result -> ladder.result로 수정하였습니다.
|
꼼꼼한 웨지…👍 추가 커멘트라니 너무 좋아요! 아, 그전에 제가 수정한 내용을 간략하게 말씀드릴게요! 이번에는 리팩터링을 크게 진행할 부분이 없어서 커밋이 별로 없네요... ㅋㅋㅋㅋ
아무튼! 저는 항상 확장성을 먼저 생각하는 편이었는데, 생각해보니 웨지의 말대로 여러 곳에서 쓰이는 것은 오히려 결합도가 높아지는 일이 되겠네요… 🤯 현재 저희가 미션을 진행하면서 중요하게 생각하는 부분은 확장성보다는, 하나의 객체는 하나의 책임을 가지도록 하는 게 더 중요하다는 걸 새겨야겠어요…! 우선 클래스가 많아져도 괜찮다는 생각으로 기존에 팩터리 메서드와 메서드 길이에 대한 커멘트도 이해했어요! 마음 속 한켠으로 불편함이 있었는데, ‘개발자의 멘탈 모델을 해치지 않는 선이라면, 굳이 바꿀 필요는 없겠다’ 라는 생각이 잡히고 나니까 괜찮을 것 같은 생각이 들었어요. ㅎㅎ 그리고 메서드 길이도 사실 고민이 많았었는데 public api만 고려하는 방법도 있었네요!! (하긴 private까지 생각한다면 100줄은 너무 엄격한 게 아닐까… 라는 생각이 들었어요… ㅎㅎ) 앞으로 개발할 때 잘 써먹을 것 같아요. 결과 처리 로직에서 다른 객체를 생성해야 한다는 걸 왜 생각을 못했을까요… 🫢 너무 좋은 솔루션이라고 생각해서, 별도의 Dto를 생성해서 처리했어요! 하지만, 로직을 구현하면서도 궁금증이 생겨서 이렇게 커멘트 달아요! ✔️ DTO로 처리하기 vs 도메인이라고 생각하기저는 DTO를 데이터를 단순히 전달하는 용도로 사용한다고 생각하기 때문에, 지금은 단순히 참가자에 대한 정보를 전달해주는 용도로 사용된다고 판단해서 DTO라는 이름을 지어주었어요. 그리고, ‘exit’라는 메시지를 받아서 처리하거나 근데, 하다 보니까 DTO가 아니라 도메인으로서도 역할을 할 수 있나…? 라는 고민이 들었어요. 이렇게 되면 컨트롤러에서 진행했던 종료 처리 로직을 도메인에게 책임을 물어서 진행할 수도 있고, 메시지도 굳이 enum으로 처리하지 않고 내부에서 상수로 관리할 수 있을 것 같다는 생각이 들었어요. 다만, 이렇게 처리하게 된다면 파라미터로 웨지라면 어떤 구조로 진행하는 게 더 나을 것 같으신가요? +) 추가적으로, boolean인 필드에 대해서 getter를 사용해야 할 때 intellij는 ✔️ 출력 형식출력 형식을 왼쪽으로 쏠리는 부분을 말씀해 주셨는데, 사실 저렇게 나오는 게 현재 사다리의 최대 폭이 5칸이기 때문에 하나의 실행 결과당 6칸씩 가질 수 있도록 (%-6s) 출력 조건을 설정해두었어요. 사다리를 구분 짓는 프레임(|)을 결과끼리 침범하지 않는 최소 선에서 출력할 수 있도록 저런 식으로 구현했어요! 실행 결과의 글자수에 따라서 동적으로 하려면 코드 자체가 이해하기 너무 어려운 로직이 될 것 같아서 (물론 구현하라고 하면 할 수 있겠지만... 뷰에 불필요하게 복잡한 로직이 추가되는 기분이어서요 😂) 제 나름대로 정책 자체를 저렇게 정해서 해두었는데, 글자 길이에 따라 동적으로 설정하는 것이 좋을까요?
미션의 예제랑 비슷한 형식으로 가져가려고 노력했는데, 수정하는 게 더 좋은 방향일지… 여쭤보고 싶습니다 ㅠㅠ 항상 좋은 피드백 주셔서 감사드립니다 🙇♀️ |
There was a problem hiding this comment.
안녕하세요 져니~!
적당히 리뷰해도 찰떡같이 받아주시니 리뷰가 편하네요.
요구사항을 다 만족한 것 같아 머지하겠습니다.
질문답변
✔️ DTO로 처리하기 vs 도메인이라고 생각하기
이 부분은 해당 예외 로직을 어느 영역까지 받아들일건지 선택의 문제라고 생각해요 ^^ 제어흐름에서 발생한 오류라고 생각하면 컨트롤러에, 참가 종료도 도메인 중 일부로 받아들일 거라면 도메인에 두시면 될 거 같아요. 두 방법 다 일리가 있어보이는데, 직접 도메인에 둬보시고 두 코드를 비교해서 고민해보시는 것도 좋겠네요.
boolean 필드의 getter 컨벤션
boolean의 경우 isXXX 가 getter 컨벤션이어서 인텔리제이가 isProceed로 만들어주는데요, 만약 변수명이 이미 is가 붙어있는 경우 생략하는 케이스입니다 ^^; (proceed로 변수명을 수정하고 다시 해보셔요)
isProceed가 맞냐 proceed가 맞냐에 대해선 거기서 거기 라고 생각해용
저는 boolean에 대해선 isXXX를 따르는 편이라 저라면 그대로 둘듯하네요.
출력 형식의 정책 변경
ㅎㅎ 어찌보면 요구사항을 지켜주신거군요? 하지만 저희가 AI와 다른 점은 나름의 기준이 있다는 것일텐데요, 자신의 도메인 지식 내에서 잘못되었다고 생각하는 부분에 대해선 의견을 내야한다고 생각합니다.
해당 정책을 만든 사람과 논의를 통해 정책을 수정할지, 혹은 감안하고 그대로 갈지 의사결정 후 진행하는 게 맞다고 봅니다. 미션 같은 경우 자기 자신이 의사결정자이기 때문에 내가 맞다고 생각하는 방향으로 진행하면 될 것 같네요
그런데 결과값 가운데 정렬이 해당 미션에서 중요한 요소는 아닌거 같아서 리뷰시엔 고려하지 않는걸로 ^_^
어려운 미션 수고하셨습니다 져니~~ 항상 거의 편지를 써주시는 져니 덕분에 저도 리뷰가 재밌었네요. 미션은 이만 머지할테니 이후 질문 있으면 따로 DM 주셔도 좋습니다 ^^
|
|
||
| import java.util.List; | ||
|
|
||
| public class ResultParticipantsDto { |
There was a problem hiding this comment.
예외 전달 객체를 잘 구현해주셨네요 ^^ 이런 객체를 리절트 반환 유형이라고 하고, 런타임 예외를 throw하는 암시적 예외 전달 방식과 달리 명시적으로 예외를 전달하는 방법 중 하나입니다. null을 반환 하는 것도 명시적으로 예외를 전달하는 방법이지만, 전 null 없는 세상이 취향이라 해당 방식을 더 선호합니다.
안녕하세요, 웨지 👋
저번 1단계 미션 때 빠르게 피드백해주신 덕분에 2단계 미션도 금방 진행할 수 있었어요. 👍 감사합니다.
이번 미션에도 어김없이 질문 사항이 있답니다 🤗...
✔️ 커밋 단위
최대한 원자적인 커밋을 진행하기 위해서 구현 기능에 대한 docs + 새로운 기능 feat + 테스트 코드 test = feat로 커밋을 진행하였습니다. 이전보다는 커밋의 수가 덕분에 좀 줄어든 것 같기는 하지만, 코드를 작성하면서 계속 리팩터링을 하다 보니까 refactor가 엄청 많아지네요... 😂 한 가지 고민인 건, 그렇다면 리팩토링의 과정까지 하나의 커밋 내에서 들어가야 하는지가 궁금해요. 수업 때 TDD를 진행할 때는 test -> 돌아가는 코드 작성 (feat) -> 그리고 리팩토링 (refactor)를 모두 포함하여 말씀해 주셨는데, 커밋 단위에서도 하나로 묶어서 진행해야 되는 건지, 아니면, 일차적으로 필요한 기능을 구현하고 나중에 한 번에 리팩터링을 진행하는 게 나을지 궁금해요.
✔️ 도메인의 예외 메시지와 테스트 코드
질문은 아니구, 저번 미션에서 말씀 주신 부분이에요! 기존에는 예외 메시지에 대해서만 public으로 열었었는데, 말씀해주신 프로덕션 코드의 유지보수 vs 테스트 코드 용이 관점으로 봤을 때 당연하게도 전자가 중요하기 때문에 private로 막고, String.format을 통해서 조금 더 관리가 용이하도록 수정해두었어요. 또한, 커스텀 예외를 여러 개 생성해도 괜찮다고 하셨지만, 너무 많아질 것 같아서 전에 사용했던 '범용적인 예외'만 두고 우선 나머지는
IllegalArgumentException으로 처리하도록 진행하였습니다!✔️ LadderFactory와 Ladder
지난 커멘트로부터 피드백을 받아서 수정한 부분이에요! 우선,
LadderFactory는 현재 Ladder의 구성품인LadderRungs를 만들고 있기 때문에 이름을LadderRungsFactory로 변경했어요. (현재 팩토리는 Ladder의 생성에 대한 복잡한 로직을 변경하기 위해서 사용하는 클래스니까요) 다만, 이 경우 Factory라는 이름을 사용해도 되는지는 고민이에요! 'LadderRungs'이라는 별도의 객체가 존재하지 않고, List에 대해 생성해주기 때문에 'Factory'라는 이름은 맞지 않다는 생각도 조금 들어서요! 🥹 우선은 생성해주는 역할이기 때문에 Factory라고 이름 지었지만, 해당 '객체'를 생성하는 것이 아니기 때문에 잘못 사용한 것인지 생각이 들었습니다.추가적으로,
LadderRungsFactory의 create 메서드의 접근 제어자를 default로 변경하였어요. protected보다 더 엄격한 제어자를 쓰는 게 더 좋을 것 같아서 사용하였어요. (상속에 대한 대비)✔️ 사다리 게임 실행 로직에 대한 고찰
저는 1단계 미션 때, 사다리 게임을 진행하는 책임을 사다리에서 진행하도록 하기 위해서
Ladder의 startGame()을 통해 진행했어요. 그리고 참여자의 시작 위치에 따라서 사다리 게임을 진행하고, 최종적으로 어느 위치에 있는지 반환하도록 만들고, 컨트롤러에서 각 참여자의 최종 위치에 대한 리스트를 생성하도록 만들었어요. (사실 제 사다리 타기 로직이 다른 협업자분이 보셨을 때 바로 이해하실 수 있을지 모르겠어요... 😂)아무튼, 구현을 완료하고 나니까, 컨트롤러에서 생성하는 최종 위치의 리스트를 생성하는 것이 LadderResult의 책임인 건가 고민이 됐어요 🤔
LadderResult는 최종 결과 리스트를 알고 있고, 리스트에 존재하는 순서가 곧 참여자의 최종 위치와 연관되기 때문에Ladder와 연관이 있는 도매인이다.Ladder와LadderResult사이의 결합도가 증가한다. 컨트롤러에서 충분히 처리한 다음 처리해줄 수 있다.제 생각에는 LadderResult는 어차피 Ladder와 연관이 있기 때문에 (사다리 결과 - 사다리) 내부에서 호출해도 괜찮을 것 같아서
1번으로 로직을 변경하기는 했는데, 현재 이 구조가 괜찮은지, 합리적인 구조인지 의문이 들어서 리뷰 요청드리고 싶어요 🙇♀️-> 생각해 보니까 컨트롤러에서 Ladder의 startGame을 호출하고, position만 넘겨주면 되겠네요 😂 (커밋 03fae40)
기존에는 자꾸 리스트를 반환하려고 하다 보니 이 구조를 생각 못한 것 같아요... ㅎㅎ
++ 위 질문에 이어서, 작성한 컨트롤러를 보니 너무 길어졌나 싶은데 별도의 컨트롤러를 생성하는 게 좋을까요? (ex. 사다리 결과에 대한 컨트롤러)
✔️ 실행 종료에 대한 로직
기능 요구사항에는 없었지만, 결과를 보고 싶은 사람을 입력받는 부분을 무한으로 처리할 수 없어서 EXIT (exit)를 입력하면 프로그램이 종료하도록 만들었어요. 다만, 'exit'는 뷰 영역의 메시지라고 생각해서 컨트롤러에서 처리하도록 진행하였는데, 이 과정에서 do-while문을 사용하였고, 종료 조건으로
getParticipants()에서 null을 반환하도록 했어요. 다만 이렇게 명시적으로 null을 반환해도 괜찮은 건가 싶어서...! (사용자의 console로는 Null 값이 들어올 수 없다고 생각했기 때문에 null로 판단하려고 했어요.) null을 반환하지 않으면 예외를 발생시키고 잡는 방법도 있을 것 같은데, 사용자의 입력에 대해 '예외'를 발생시키는 건 또 좋은 방법은 아닌 것 같아서, 혹시 이 부분에 대해서 조언 주실 수 있을까요?이번 미션은 조금 더 로직적인 질문이 많아진 느낌이네요...ㅎㅎㅎㅎ
이번 리뷰도 잘 부탁드립니다 🙇♀️ 항상 감사드려요!