🚀 3단계 - 사다리(게임 실행) #2418
🚀 3단계 - 사다리(게임 실행) #2418
Conversation
…lass with interface
# Conflicts: # src/main/java/nextstep/Main.java # src/main/java/nextstep/view/InputView.java # src/main/java/nextstep/view/OutputView.java
|
#tag @neojjc2 |
|
|
||
| out.printLadder(names, ladder, bonus); | ||
| while (true) { | ||
| String target = in.readTarget().trim(); |
There was a problem hiding this comment.
target이 원시객체로 포장되면 어떨까요?? 😄
어찌보면 사다리게임에서 사용되는 커맨드역할이기 때문에 도메인모델로 표현되도 괜찮을 것 같습니다.
// 뼈대만 있는 코드인점 감안하고 봐주시면 감사하겠습니다 :-)
LadderGameCommand command = LadderGameCommand.parseBy(in.readTarget());
if (command.isAll()) {
// 기존코드
}
Name name = Name.of(command.getValue());그럼 만약에 exit나 이름이 아닌 index 등등 커맨드관련해서 수정사항이 발생했을 때 관리도 편하고
"all"이 아니라 전체를 보여달라는 명령이 변경되도 기존 코드 수정 없이 LadderGameCommand 객체만
수정하면 될 것 같습니다 😄
| break; | ||
| } | ||
| Name n = Name.of(target); | ||
| int idx = names.unmodifiableNames().indexOf(n); |
There was a problem hiding this comment.
전체적으로 index기준으로 핸들링이 되고 있는데요, List이니 당연히 그럴 수 밖에 없는점이라 생각합니다 😄
도메인 객체를 잘 정의하고 역할과 책임을 부여하는 게 가장 중요하지만, 개인적으로 그 다음은
사용하는 다른 객체들이나 서비스들에게 편리한 인터페이스를 제공하는 것이라고 생각합니다.
Name을 Participant같은 객체로 상향(?) 시키고 생성할 때
그냥 index값을 position같은 값으로 가지고 있으면 어떨까요??
/**
* 이름 클래스
*/
public class Participant {
private static final Integer MAX_NAME_LENGTH = 5;
private final String name;
private final int position;그리고 Names도 Participants등으로 변경되고 findByName(Participant) 같은
method를 Optional로 제공한다면 이름이 없을 수도 있는 상황에 대해서 명시적으로 표현 될 것 같습니다.
Optional<Participant> searched = participants.findByName(participant);사용하는 쪽에서 0이나 -1은 통상적으로 통용되는 의미이긴 합니다만
사실 관리하기 번거로운 값입니다. 그리고 매번 값이 없는 상황이 -1인지 코드를 따라가면서
확인해야 하는 값입니다 😄
물론 예전 레거시코드들은 아직도 -1이면 값이 없는 상태로 표현이 되어있습니다만
버전을 거듭하면서 Optional이나, Exception으로 개선되고 있는 형태로 가독성이나 사용성을
개선해가는 형태라서 그 부분도 한번 참고해주시면 좋을 것 같습니다.
추가적으로 디미터 법칙이 있는데 같이 한번 보시면 좋을 것 같습니다 😄
| public String toString() { | ||
| return bonusName; | ||
| } | ||
| } |
| private final Map<Integer, Integer> result; | ||
|
|
||
| public LadderResult(Ladder ladder) { | ||
| result = ladder.result(); |
There was a problem hiding this comment.
위에서도 의견드렸지만 Map<Integer, Integer> 형태는 사용하는 쪽에서는 어떤 값이 들어있는지
명시적으로 알 수 없기 때문에 매번 이쪽에 와서 무슨 값이 들어가는지 확인해야 하는 코드 입니다.
그리고 Integer라면 무슨 값이든 들어갈 수 있기 때문에 타입안정성 측면에서도 체크 되는게
좀 더 안전할 것 같습니다 😄
Map<Participant, Bonus> 이런식으로 제공되면
index로 물어보는게 아니라 LadderResult에게 이름을
주면 바로 Bonus를 알 수 있을 것 같은데 어떻게 보시나요 😄
| private void validate(String input) { | ||
| if (input == null || input.trim().isEmpty()) { | ||
| throw new IllegalArgumentException("이름은 빈 문자열이 될 수 없습니다."); | ||
| } | ||
| if (input.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("이름은 최대 " + MAX_NAME_LENGTH + "자까지 허용됩니다."); | ||
| } | ||
| if (input.equals("all")) { | ||
| throw new IllegalArgumentException("사용할 수 없는 이름입니다."); | ||
| } |
There was a problem hiding this comment.
위에서도 의견 드렸지만, Name과 Command를 분리하면 사용할 수 있는 이름이 됩니다 😄
ALL이라는 이름을 가진 사람이 있을 수도 있습니다. 이름을 돌려주시죠 😆
| throw new IllegalArgumentException("보너스 개수는 사람 수와 일치해야 합니다."); | ||
| } | ||
| return bonus; | ||
| } |
There was a problem hiding this comment.
검증 코드 너무 좋은데 OutputView에 있으면 안될 것 같습니다.
Bonus도 일급컬렉션으로 가고 그쪽에 위임하시는걸 추천 드립니다 🙇
| Name name = names.unmodifiableNames().get(i); | ||
| Bonus bonus = bonuses.get(result.get(i)); | ||
| System.out.printf("%s : %s%n", name, bonus); | ||
| } |
There was a problem hiding this comment.
위에서도 의견드렸지만, Participant(=Name)가 name과 index를 관리하고 있다면
participants(=names)를 순회하면서 바로 결과를 가져올 수 있을 것 같습니다 😄
안녕하세요 리뷰어님, 좋은 피드백 덕분에 많은 걸 배우고 있는것 같습니다!
이번에도 많은 피드백 부탁드려요. 감사합니다!
+) Conflict가 발생해서 인라인 수정 거쳤습니다!