[LBP] 권지후 사다리 2-5단계 미션 제출합니다 #44
Conversation
There was a problem hiding this comment.
2~5단계 미션 고생하셨습니다 👍 👍
좀 더 적극적으로 객체 간의 협력을 도전해보면 좋을 것 같아요.
모든 값들을 포장만 해두고, Game이나 Controller에서 그것들을 꺼내어 작업하려는 부분이 많았던 것 같아요.
너무 현실처럼 생각하지 않으면서, 이 필드를 가지고 있는 애한테 시켜볼까?
라는 접근으로 도전해보시면 좋을 것 같아요.
예를 들어, Ladder가 플레이어와 실행 결과를 가지면서, 스스로 계산을 할 수 있게 만든다면, Game 같은 객체를 없앨 수 있지 않을까요? 객체 간의 협력을 적극적으로 활용해보는 시간이면 더 좋을 것 같아요.
View 나누는 기준
추가로 지후님은 InputView와 ResultView로 나누는 기준이 있으신가요?
요구사항에 있어서 그럴 수도 있지만 지후님만의 근거가 있으면 더 좋을 것 같아요.
키오스크를 예시로 생각했을 때, 키오스크는 입력과 출력으로 나뉜 것이 아니라
각 단계별로 화면이 구성되어 있어요. 이럴 때도, 입력과 출력으로 나누면 좋을까요?
- 뷰가 모델을 최대한 모르게 하시려고, 문자열, List, Map 등 모든 가공이 컨트롤러 또는 모델 객체에서 이루어지고 있는 것 같아요.
- 뷰가 모델을 알면 안되는 이유는 무엇일까요?
그렇다던데가 아닌 지후님이 고민하고 결정하는 과정이 있으면 더 유의미한 시간일 것 같습니다.
질문 답변
최대한 원시값을 포장하였는데 제가 놓친 부분이 있을까요?
크게 없는 것 같습니다. 다만, 그것으로 느낀 단점이 있을까요?
지후님은 요구 사항이 없더라도 원시값 포장을 준수하실 건가요?
저번 리뷰에서 받았던 것처럼 생성자에서 모든 처리를 하는 방식 대신,
정적 팩토리 메서드로 리팩토링했는데,
적절한 리팩토링이라고 생각하시는지 궁금합니다.
드라마틱한 변화는 느끼지 못했으나, 이 부분은 메서드 명을 좀 더 명확히 해주는 것으로 해결해볼 수 있을 것 같네요. 정적 팩터리 메서드의 장점인 이름을 지어줄 수 있다!를 활용하지 못한다는 느낌을 받았습니다.
저는 직접 필드에 초기화 시키는 것이 더 간결하고 직관적이라 생각해서 그동안 이렇게 구현해왔습니다..!
혹시 생성자에서 초기화 시키는 게 더 이점이 있어서 질문 주신 걸까요?
어디서는 주입, 어디서는 필드 초기화 방식을 사용하셔서 여쭤본 거였습니다! 근거가 있다니까 좋은 것 같네요. 👍 또한, 저도 View나 Controller 부분의 테스트는 가성비가 나오지 않아서 직접 초기화 하는 방식을 사용해도 큰 문제는 없을 것 같다고 생각해요.
| import java.util.List; | ||
| import java.util.Scanner; | ||
|
|
||
| public class InputView { |
There was a problem hiding this comment.
의도된 결과일까요?
- 참여할 사람은
,가 3개로 분리 (4명의 이름으로 분리될 것을 예상) - 실행 결과는
,가 2개로 분리 (3개의 결과로 분리될 것을 예상)
There was a problem hiding this comment.
LMS에서 나온 결과값으로 계속 입력하며 실행 결과를 확인하다 보니 미처 그 부분을 놓쳤습니다.
플레이어 수에 따라 사다리가 정상적으로 생성되게, 그리고 유효성 검사를 추가해 빈 값이 들어올 경우
예외를 던지도록 리팩토링을 진행했습니다. 감사합니다!
src/main/java/model/Height.java
Outdated
| public class Height { | ||
| private static final int MINIMUM_HEIGHT = 2; | ||
|
|
||
| private final int value; | ||
|
|
||
| public Height(int value) { | ||
| validateValue(value); | ||
| this.value = value; | ||
| } | ||
|
|
||
| private void validateValue(int value) { | ||
| if (value < MINIMUM_HEIGHT) { | ||
| throw new IllegalArgumentException("사다리 높이는 2 이상이여야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
예외 메시지도 상수를 사용하는 것에 대해서는 어떻게 생각하시나요?
There was a problem hiding this comment.
ExceptionMessage 열거형 클래스를 생성해
예외메세지를 통합 관리하게 리펙토링 하였습니다 감사합니다 !
package exception;
public enum ExceptionMessage {
LADDER_HEIGHT_MIN_VALUE("사다리 높이는 최소 2 이상이어야 합니다."),
LADDER_SIZE_NEGATIVE("사다리 사이즈가 0보다 작을 수는 없습니다."),
LADDER_HEIGHT_NOT_NUMBER("사다리 높이는 숫자여야 합니다."),
PLAYER_NAME_MAX_LENGTH_EXCEEDED("참가자 이름은 최대 5글자를 초과할 수 없습니다."),
MIN_PLAYERS_REQUIRED("참가자는 최소 2명 이상이여야 합니다."),
RESULT_COUNT_MISMATCH("실행결과 개수와 참가자의 수는 동일해야 합니다."),
RESULT_NOT_NULL_OR_EMPTY("실행결과는 null이거나 공백일 수는 없습니다."),
NULL_OR_EMPTY_INPUT("입력값이 null이거나 비어있을 순 없습니다.");
private final String message;
ExceptionMessage(String message) {
this.message = message;
}
public String getMessage() {
return message;
}
}There was a problem hiding this comment.
엇 제가 의도했던 것은
public class Height {
private static final int MINIMUM_HEIGHT = 2;
private final int value;
public Height(int value) {
validateValue(value);
this.value = value;
}
private void validateValue(int value) {
if (value < MINIMUM_HEIGHT) {
throw new IllegalArgumentException("사다리 높이는 %d 이상이여야 합니다.".formatted(MINIMUM_HEIGHT));
}
}처럼 변경될 수 있는 조건을 상수로 선언했던 것을 활용하는 것에 관한 이야기였어요!
There was a problem hiding this comment.
별개로 예외 메시지를 Enum으로 관리했을 때의 장점은 무엇인가요?
src/main/java/model/Players.java
Outdated
|
|
||
| public class Players { | ||
|
|
||
| private static final int MITMUM_PLAYER_LENGTH = 2; |
There was a problem hiding this comment.
| private static final int MITMUM_PLAYER_LENGTH = 2; | |
| private static final int MINIMUM_PLAYER_LENGTH = 2; |
There was a problem hiding this comment.
오타낸 부분이 많네요.. ㅠㅠ 더 꼼꼼히 보겠습니다 감사합니다
|
|
||
| public class Player { | ||
|
|
||
| private static final int MAX_NAME_LENGTH = 5; |
There was a problem hiding this comment.
MINIMUM의 줄임말로 MIN을 사용하고, MAXIMUM의 줄임말로 MAX를 사용하는 것 같아요.
그러나 현재 프로젝트에서는 MAX도 있고 MINIMUM도 쓰이고 있어요.
표현 방식을 통일하는 것은 어떨까요?
There was a problem hiding this comment.
그때그때 떠오르는 대로 변수명을 선언했는데, 표현 방식을 더 통일해보겠습니다
ExceptionMessage 클래스 내에서 표현통일해서 다시 네이밍 리펙토링 하였습니다 감사합니다
| public class FixedNumberGenerator implements RandomUtil { | ||
| private final int fixedNumber; | ||
|
|
||
| public FixedNumberGenerator(int fixedNumber) { | ||
| this.fixedNumber = fixedNumber; | ||
| } | ||
|
|
||
| @Override | ||
| public int generateRandomNumber() { | ||
| return fixedNumber; | ||
| } | ||
| } |
There was a problem hiding this comment.
test를 위한 구현체는 테스트 폴더 하위에 두면 어떨까요?
실제 어플리케이션을 실행할 때는 필요하지 않을 것 같아서요. 😄
학습 키워드
- 빌드할 때 test 폴더 내부의 파일도 함께 빌드 되는가?
- 아니라면, 테스트를 위한 코드는 test 내부에?
There was a problem hiding this comment.
단순히 프로덕션 코드에 test 패키지를 생성해 넣어두었는데,
애플리케이션 실행 시 test 폴더에 있는 파일은 기본적으로 빌드 대상에 포함되지 않는다는 것을 알게 되어,
테스트 폴더 내 util 패키지로 클래스를 이동했습니다. 좋은 피드백 감사드립니다!
| Players player = new Players(inputView.inputNames()); | ||
| Prizes prizes = Prizes.form(inputView.inputResult(), player); | ||
| Height height = new Height(inputView.getMaxLadderHeight()); |
There was a problem hiding this comment.
Players 타입의 변수명에 player -> players로 작성하면 더 좋을 것 같아요.
변수명만 보고 단일 Player인 줄 알았습니다..!
Prizes.form(입력 메서드로 가져온 무언가, playe); // 무엇으로 어떤 Prizes를 만드는지 이해하기 어려워요.
정적 팩터리 메서드 명을 더 명확하게 작성해주면 좋을 것 같아요!
There was a problem hiding this comment.
다른 데 신경 쓰다 보니 변수명을 놓친 것 같습니다 ㅠㅠ
players로 리팩토링 진행하였고, form 또한 오타여서 createPrizes로 네이밍 수정하였습니다.
앞으로는 정적 팩토리 메서드의 장점인 의미 있는 이름을 지을 수 있다는 점을 더 잘 활용해 보겠습니다. 감사합니다!
| @@ -1,26 +1,43 @@ | |||
| package controller; | |||
|
|
|||
| import model.*; | |||
There was a problem hiding this comment.
제가 의도하지 않은 부분 같습니다..
확인해봤더니 클래스를 import할 때, 자동으로 와일드카드(*)로 통합되는 경우인거 같은데,
인텔리제이 내에서 각각의 클래스들을 명시적으로 import 문에 써주도록 설정해두겠습니다 감사합니다 !
src/test/java/model/HeightTest.java
Outdated
| @Test | ||
| @DisplayName("Height 객체의 높이가 2 미만일 때 예외가 발생하는지 검증한다.") | ||
| void shouldThrowExceptionForHeightBelowMin() { | ||
| assertThatThrownBy(() -> new Height(1)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("사다리 높이는 2 이상이여야 합니다."); | ||
| } |
There was a problem hiding this comment.
| @Test | |
| @DisplayName("Height 객체의 높이가 2 미만일 때 예외가 발생하는지 검증한다.") | |
| void shouldThrowExceptionForHeightBelowMin() { | |
| assertThatThrownBy(() -> new Height(1)) | |
| .isInstanceOf(IllegalArgumentException.class) | |
| .hasMessage("사다리 높이는 2 이상이여야 합니다."); | |
| } | |
| @Test | |
| @DisplayName("높이가 2 미만일 때 예외가 발생한다.") | |
| void shouldThrowExceptionForHeightBelowMin() { | |
| assertThatThrownBy(() -> new Height(1)) | |
| .isInstanceOf(IllegalArgumentException.class) | |
| .hasMessage("사다리 높이는 2 이상이여야 합니다."); | |
| } |
클래스명이 바뀌더라도 테스트의 @DisplayName이 바뀌지 않으며,
예외가 발생하는 것을 테스트하기 때문에, 예외가 발생한다. 까지만 작성해도 충분할 것 같은데
어떻게 생각하시나요?
There was a problem hiding this comment.
네! 제가 테스트 @DisplayName 컨벤션을 -지 검증한다로 통일해서 작성하였는데,
같은 의미의 반복이라면 말씀해주신대로 더 간결하게 작성해도 충분할 것 같네요
리팩토링 완료했습니다. 감사합니다!
src/test/java/model/PrizeTest.java
Outdated
| @Test | ||
| @DisplayName("값이 null이면 예외를 발생시키는지 검증한다.") | ||
| public void shouldThrowExceptionWhenValueIsNull() { | ||
| String invalidValue = null; | ||
|
|
||
| assertThatThrownBy(() -> new Prize(invalidValue)) | ||
| .isInstanceOf(IllegalArgumentException.class) | ||
| .hasMessage("실행결과는 null이거나 공백일 수는 없습니다."); | ||
| } |
There was a problem hiding this comment.
| @Test | |
| @DisplayName("값이 null이면 예외를 발생시키는지 검증한다.") | |
| public void shouldThrowExceptionWhenValueIsNull() { | |
| String invalidValue = null; | |
| assertThatThrownBy(() -> new Prize(invalidValue)) | |
| .isInstanceOf(IllegalArgumentException.class) | |
| .hasMessage("실행결과는 null이거나 공백일 수는 없습니다."); | |
| } | |
| @ParameterizedTest | |
| @NullAndEmptySource | |
| @DisplayName("값이 null이면 예외를 발생시키는지 검증한다.") | |
| public void shouldThrowExceptionWhenValueIsNull(String invalidValue) { | |
| assertThatThrownBy(() -> new Prize(invalidValue)) | |
| .isInstanceOf(IllegalArgumentException.class) | |
| .hasMessage("실행결과는 null이거나 공백일 수는 없습니다."); | |
| } | |
아래의 빈문자열 테스트와 합쳐서 이렇게도 쓸 수 있을 것 같아요!
There was a problem hiding this comment.
좋은 정보 감사합니다!
보여주신 코드가 훨씬 깔끔하고 직관적인 것 같습니다.
테스트에서도 @NullAndEmptySource나 @ValueSource 같은 아노테이션을 더 활용해 보겠습니다.
감사합니다!
| public void calculateResults(List<String> playerNames, Prizes prizes) { | ||
| List<Line> lines = ladder.getLines(); | ||
| List<String> prizeValues = prizes.getPrize(); | ||
|
|
||
| for (String playerName : playerNames) { | ||
| int playerIndex = playerNames.indexOf(playerName); | ||
| for (Line line : lines) { | ||
| playerIndex = getNewIndexMove(playerIndex, line.getPointGroups()); | ||
| } | ||
| results.put(playerName, prizeValues.get(playerIndex)); | ||
| } | ||
| } |
There was a problem hiding this comment.
indent는 1까지만 허용한다. 요구사항을 지키지 못한 가장 큰 이유가 지후님은 무엇이라고 생각하시나요?
There was a problem hiding this comment.
구현 로직에 집중하다 보니.. 들여쓰기를 많이 사용한 것 같습니다.
앞으로는 요구사항을 준수하면서 코드 작성할 때, 인덴트를 1칸으로 유지하도록 주의하겠습니다
아래와 같이 리펙토링 진행하였습니다 감사합니다.
public void calculateResults(List<String> playerNames, Prizes prizes) {
List<String> prizeValues = prizes.getPrize();
for (String playerName : playerNames) {
int playerIndex = playerNames.indexOf(playerName);
playerIndex = ladder.move(playerIndex);
results.put(playerName, prizeValues.get(playerIndex));
}
}
사다리가 제대로 생성되지 않는 것 같아서 확인 부탁드려요~ |
각 객체에 어떤 책임을 맡기고 위임해야 할지, Q : |
저는 입력값만 받는 |
저는 실제로 코드를 구현해보면서도 모델이 변경되면 가공하는 컨트롤러는 변경되지만 뷰는 영향을 받지 않는다는 이점을 느꼈기도 하였고, |
준수할 것 같습니다 ! |
There was a problem hiding this comment.
Q :예혁님은 설계를 할 때 먼저 어떻게 생각 하시고 코드 로직을 구현하시는 지 여쭤봐도 될까요?
음, 객체간의 협력을 고민한다고 해볼게요.
로또 게임에서 구입한 로또와 당첨 로또를 비교한다고 할 때, 누가 할 수 있는 일일까요?
현실에서는 아래처럼 누군가 판단해줘야 한다고 생각할 수 있을 것 같아요.
int matchCount = judgement.getMatchCount(lotto, winningLotto);저는 근데 로또가 스스로 번호들을 가지고 있으니, 스스로 할 수 있지 않을까? 라는 고민을 해요.
int matchCount = lotto.countMatchCount(winningLotto);그 상태를 가지고 있는 객체가 책임지고 수행하려고 해보면 그 사이에서 좋은 방법을 찾을 수 있을 것 같아요.
| import static model.Point.HAS_POINT; | ||
|
|
||
| public class LadderController { | ||
|
|
||
| private static final int LADDER_SIZE = 4; | ||
| private static final int CHUNK_SIZE = 3; | ||
| private static final String ALL_PLAYERS = "all"; |
There was a problem hiding this comment.
"all" 이라는 문자열 리터럴이 모든 플레이어들을 나타낸다고 생각하니 어색한 것 같아요. 어떻게 생각하시나요?
There was a problem hiding this comment.
미션 LMS에 실행결과에서의 예시가 all로 명시되어 있어
all 이라는 문자값을 넣어주되 상수 명을 ALL_PLAYERS라고 명시해줬습니다 !
| public enum ExceptionMessage { | ||
| LADDER_HEIGHT_MIN_VALUE("사다리 높이는 최소 2 이상이어야 합니다."), | ||
| LADDER_SIZE_NEGATIVE("사다리 사이즈가 0보다 작을 수는 없습니다."), | ||
| LADDER_HEIGHT_NOT_NUMBER("사다리 높이는 숫자여야 합니다."), | ||
|
|
||
| PLAYER_NAME_MAX_LENGTH_EXCEEDED("참가자 이름은 최대 5글자를 초과할 수 없습니다."), | ||
| MIN_PLAYERS_REQUIRED("참가자는 최소 2명 이상이여야 합니다."), | ||
|
|
||
| RESULT_COUNT_MISMATCH("실행결과 개수와 참가자의 수는 동일해야 합니다."), | ||
| RESULT_NOT_NULL_OR_EMPTY("실행결과는 null이거나 공백일 수는 없습니다."), | ||
| NULL_OR_EMPTY_INPUT("입력값이 null이거나 비어있을 순 없습니다."); |
There was a problem hiding this comment.
좋은 시도인 것 같아요. 다만 바뀔 수 있는 규칙에 대해서는 고민해보면 좋을 것 같아요! 👍
예: 사다리 최소 높이가 3으로 변경되었을 때, 이 메시지에서도 변경해줘야 한다.
There was a problem hiding this comment.
이 부분은 String.format() 와 같은 포멧팅을 사용하여 메세지를 동적으로 관리할 수 있을 것 같습니다
앞으로 규칙값이 변경될 가능성을 염두에 두고 포멧팅 방법 등을 참고해보겠습니다.
좋은 피드백 감사합니다 !
| public class LadderResult { | ||
| private static final String NO_RESULT = "결과 없음"; | ||
| private final Map<String, String> results; | ||
| private final Ladder ladder; |
There was a problem hiding this comment.
도메인, 출력
어디에 더 가까운가요?
무엇을 책임지고 있나요?
There was a problem hiding this comment.
도메인 클래스라고 생각합니다
결과를 포맷팅해서 출력하는 역할이 없고,
결과를 계산하는 비즈니스 로직을 책임지고 있기 때문에
LadderResult는 도메인 클래스라고 생각합니다.
| private List<Player> generatePlayers(List<String> players) { | ||
| return players.stream() | ||
| .map(Player::new) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
| private List<Player> generatePlayers(List<String> players) { | |
| return players.stream() | |
| .map(Player::new) | |
| .toList(); | |
| } | |
| private List<Player> generatePlayers(List<String> playerNames) { | |
| return playerNames.stream() | |
| .map(Player::new) | |
| .toList(); | |
| } |
더 명확히 써볼 수 있을 것 같아요. 😄
There was a problem hiding this comment.
말씀하신 것처럼 코드 작성 시에는 제가 제 코드를 가장 잘 알기 때문에
"이 정도면 되겠지" 하고 넘아가곤 했는데,
협업이나 현업에서는 다른 사람들이 코드를 읽을 가능성도 있으므로
더 많은 사람들과 코드를 공유할 때를 고려해서 가독성 좋게 코드를 작성하려고 노력하겠습니다
피드백 주셔서 감사드립니다 !
| public class Player { | ||
|
|
||
| private static final int MAX_NAME_LENGTH = 5; | ||
| private final String value; |
There was a problem hiding this comment.
이 피드백 받고 저도 value가 뭐였지,, 하면서 다시 코드를 살펴보는 것 자체가
네이밍이 명확치 않아서 생긴 문제라고 생각되네요ㅜㅜ
플레이어 이름을 나타내는value 변수명이 명확하지 않아, playerName으로
다시 네이밍하는 것이 더 직관적이고 명확하다고 생각됩니다. 감사합니다
| public interface RandomUtil { | ||
| int generateRandomNumber(); | ||
| } |
| assertThat(result).hasSize(4); | ||
| assertThat(result.get(0)).isEqualTo(Point.HAS_POINT); | ||
| assertThat(result.get(1)).isEqualTo(Point.NO_POINT); | ||
| assertThat(result.get(2)).isEqualTo(Point.NO_POINT); | ||
| assertThat(result.get(3)).isEqualTo(Point.HAS_POINT); |
[LBP] 초록스터디
리뷰어: 방예혁님
리뷰이: 권지후
사다리 남은 단계를 구현하면서 느낀점
사다리 남은단계 미션을 제출합니다!
이번 미션에서는 사다리 결과값과 참가자를 어떻게 매칭해야 하는지가
가장 고민되고 어려웠던 부분이었습니다.
그래서 여러 코드를 서치하고 참고하여 코드를 구현하게 되었습니다.
저번 리뷰에서 받은 피드백을 반복하지 않으려고 노력했지만,
구현에 집중하다 보니
getter가 조금 남발된 부분이 있다고 생각됩니다...ㅠㅠ리펙토링 때 이 부분도 더 고민해서 같이 리펙토링 진행하겠습니다.
바쁘실 텐데 코드리뷰 해주셔서 감사드립니다.
🪜 사다리 미션 구조
Model
Enum) 클래스 (HAS_POINT또는NO_POINT)View
|), 연결된 가로줄(-----), 연결되지 않은 공백을 표현하는 열거형(Enum) 클래스Test
RandomUtil테스트 클래스용 구현체Controller
❓ 질문
Q1
최대한 원시값을 포장하였는데 제가 놓친 부분이 있을까요?
Q2
저번 리뷰에서 받았던 것처럼 생성자에서 모든 처리를 하는 방식 대신,
정적 팩토리 메서드로 리팩토링했는데,
적절한 리팩토링이라고 생각하시는지 궁금합니다.
저번 리뷰 질문 답변
저는 직접 필드에 초기화 시키는 것이 더 간결하고 직관적이라 생각해서 그동안 이렇게 구현해왔습니다..!
혹시 생성자에서 초기화 시키는 게 더 이점이 있어서 질문 주신 걸까요?