[2단계 - 사다리 게임 실행] 조이(김성연) 미션 제출합니다. #225
Conversation
|
리뷰에 시간이 조금 걸릴 것 같아 질문에 대한 답변 먼저 하겠습니다 😊
먼저 Validator가 꼭 필요한 지부터 고민해보면 좋겠습니다. Validator가 검증하고 있는 것들 중엔 각 객체가 생성될 때 필요한 조건들이 있는데요, 이걸 객체 내부에서 검증할 순 없는 걸까요? Validator로 검증을 할 때와 객체에서 검증을 할 때의 차이는 뭐가 있는지 한번 알아보는 건 어떨까요? (+) 코드를 보니 Name, Rewards에서는 검증을 객체에서 해주고 있는데, 검증 방식은 통일해주세요!
이 미션에서는 적절하게 구현해주신 것 같습니다 👍 그런데 왜 생성자를 통해 넣는 게 어색한 걸까요? 둘 다 같은 값을 넣어 객체를 생성하는 건 똑같은데 말이에요. 🤔 이유와 함께 생성자, 정적 팩토리 메서드의 차이와 각각의 장단점을 정리해보면 좋을 것 같습니다. (+) 정적 팩토리 메서드는 명명방식에 따라 사용하는 방식이 달라지는 부분이 있는데요. 그것도 한번 찾아보면 좋을 것 같습니다. 그 외에도 이펙티브 자바를 읽으면 도움 되는 내용이 많으니, 시간 날 때 읽어보는 걸 추천드립니다. 😎
빠르게 훑어봤을 때 디미터 법칙도 없고, 크게 눈에 띄는 점은 없었습니다. (제대로 리뷰하면서 의견이 달라질 수도 있습니다ㅋㅋ) 그럼에도 불구하고 get~ 메서드가 많이 보이는 것은 사실입니다. 그럼 getter를 어떻게 줄일 수 있을까요? 엘레강트 오브젝트에서는 getter/setter 안티 패턴에서 유해한 부분은 두 접두사인 get과 set이라고 합니다. 이 접두사들 때문에 객체가 진짜 객체가 아닌 데이터 저장소로 취급 받는다는 거죠. 책에서 내놓은 해결책은 간단합니다. 접두사를 제거하는 겁니다.
위 방법이 정답은 아니니 꼭 적용할 필요는 없습니다만, get이라는 접두사는 어디에 붙여야 가장 적절한지에 대해 고민해보면 좋을 것 같습니다. |
src/main/java/Main.java
Outdated
| @@ -1,11 +1,12 @@ | |||
| import controller.LadderGameController; | |||
| import domain.InputValidator; | |||
| import view.InputView; | |||
| import view.OutputView; | |||
|
|
|||
| public class Main { | |||
| public static void main(String[] args) { | |||
There was a problem hiding this comment.
현재 execute 함수에서 illegalArgumentException 만 잡고있는데요, 가장 상위 레벨에서 catch를 하고 있기 때문에 예상치 못한 에러를 대비해 타입을 Exception 으로 바꾸는 게 나을 것 같습니다.
There was a problem hiding this comment.
커밋이 동일한 메세지명이 있어서 살펴보니 다른 파일들을 같은 이유로 변경했던데, 원격 저장소에 아직 push를 안 한 상태라면 터미널에서 git commit --amend 를 쓰거나 인텔리제이에서 커밋 수정 기능을 활용해 직전 커밋에 현재의 변경 사항을 반영할 수 있습니다! (push를 했다면 추후 force push를 해야 하니 참고 바랍니다.)
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
|
|
||
| class InputValidatorTest { |
| @@ -21,31 +26,83 @@ public LadderGameController(InputView inputView, OutputView outputView) { | |||
| } | |||
|
|
|||
| public void run() { | |||
There was a problem hiding this comment.
하나라도 틀리면 얄짤없이 프로그램이 종료되는군요... 🥹(필수는 아니기 때문에 꼭 반영할 필요는 없습니다!)
There was a problem hiding this comment.
저도 필요한 기능이라고 생각되어 이번 리팩터링에 추가해보았습니다
| private List<User> generateUsers(List<String> userNames) { | ||
| return userNames.stream() | ||
| .map(Name::new) | ||
| .map(User::new) | ||
| .collect(Collectors.toUnmodifiableList()); | ||
| List<User> users = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < userNames.size(); i++) { | ||
| users.add(new User(new Name(userNames.get(i)), new Position(i))); | ||
| } | ||
|
|
||
| return users; | ||
| } |
There was a problem hiding this comment.
생성자 오버로딩을 사용하면 굳이 컨트롤러에 둘 필요가 없을 것 같습니다. 😎
There was a problem hiding this comment.
생성자 오버로딩을 사용하려다가 아래 코멘트를 참고하여 정적 팩토리 메서드를 적용하였습니다.
- 전체 코드 스타일 통일 2. 객체 생성 캡슐화 가 주된 이유입니다.
src/main/java/domain/User.java
Outdated
|
|
||
| public User(Name name) { | ||
| public User(Name name, Position position) { |
There was a problem hiding this comment.
어떤 건 정적 팩토리 메서드를 쓰고 어떤 거 아닌데 기준이 있을까요?
There was a problem hiding this comment.
정적 팩토리 메서드를 적용한 객체들은 생성자에 해당 객체가 알 필요가 없는 인자가 들어간다고 생각해서 사용했습니다. 그런데 이번 리팩터링 과정에서 다시 정적 팩토리 메서드에 대해서 공부해보고 생각이 변하게 되었습니다.
정적 팩토리 메서드의 이점은 많지만 해당 미션에서 제가 생각한 이점은 2가지 입니다.
1. 정적 팩토리 메서드를 사용하게 되면 객체를 생성하는 메서드에 네이밍을 할 수 있습니다.
- 저는 네이밍 컨벤션을 적용하였습니다.
- 하나의 매개변수를 받아 객체를 생성하는 경우
from으로, 여러개의 매개 변수를 받아서 객체를 생성하는 경우of로 이름을 작성하였습니다.
2. 객체 생성을 캡슐화 할 수 있다.
- 전체 도메인 객체 생성에 정적 팩토리 메소드를 적용한 이유는 코드의 통일성을 고려한 점도 있지만 해당 이점을 크게 생각했습니다. 생성자를 단순화 시킬 수 있고, 은닉할 수 있습니다.
추천해주신대로 이펙티브 자바의 정적 팩토리 메소드 부분을 읽어보았습니다. 아직 제가 부족함이 많아 바로 이해는 못했습니다.. 그래서 Tecoble을 추가로 참고하여 학습해 보았습니다!
혹시 제가 놓친 부분이 있다면 말씀주세요!
There was a problem hiding this comment.
잘 적용해주셨습니다. 👏
정적 팩토리 메서드는 잘 응용하면 싱글턴 패턴으로 사용할 수도 있으니 참고 바랍니다. 😉
| public enum LadderConnectionStatus { | ||
| CONNECTION("-----"), | ||
| NON_CONNECTION(" "); | ||
|
|
| private void validateRewardLengthByMaximumLimit(String reward) { | ||
| if (reward.length() > MAXIMUM_REWARD_LENGTH_LIMIT) { | ||
| throw new IllegalArgumentException( | ||
| String.format( | ||
| INVALID_REWARD_LENGTH_BY_MAXIMUM_LIMIT.getMessage() | ||
| , MAXIMUM_REWARD_LENGTH_LIMIT)); | ||
| } | ||
| } | ||
|
|
||
| private void validateRewardLengthByMinimumLimit(String reward) { | ||
| if (reward.length() < MINIMUM_REWARD_LENGTH_LIMIT) { | ||
| throw new IllegalArgumentException( | ||
| String.format( | ||
| INVALID_REWARD_LENGTH_BY_MINIMUM_LIMIT.getMessage(), | ||
| MINIMUM_REWARD_LENGTH_LIMIT)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
숙지하고 진행했다고 생각했는데 코드를 작성하면서 다시 돌아가 버렸네요.. 이전에 리뷰해주신 내용 다시 읽어보았습니다. 고민하다가 최대 값, 최소 값에 대한 검증을 굳이 분리해서 할 필요가 없다고 느껴 합쳤습니다.
주요한 이유는 1. 메서드를 통합하여 가독성을 높이고 2. 사용자가 값을 잘못 입력했을 때 한번에 입력값의 요구사항을 파악할 수 있도록 하는 것입니다.
위와 같은 이유로 길이, 범위에 대한 검증 메서드는 모두 통합 하였습니다. 혹시 주신 코멘트에서 의도하신 것이 이러한 변경이 아니라면 말씀주세요!
src/main/java/domain/LadderGame.java
Outdated
| return connections; | ||
| } | ||
|
|
||
| private void addIndex(boolean point, List<Integer> connections, int i) { |
There was a problem hiding this comment.
getConnection의 경우 for문에서 쓰이는 인덱스(=보편적인 약속)이기에 약자로 써도 되지만 addIndex에서는 for문에서 온 인덱스라는 정보가 없으므로 약자로 쓰지 않고 index라고 명시해 주는 게 좋습니다.
There was a problem hiding this comment.
메소드 추출 기능을 써서 하다가 변수명을 놓친 것 같습니다. 감사합니다 춘식!
src/main/java/domain/Name.java
Outdated
| private final String name; | ||
|
|
||
| public Name(String name) { | ||
| name = name.trim(); | ||
| name = name.strip(); |
…ption 예외를 발생시키도록 수정
|
안녕하세요 춘식! 🐱 지난 리뷰를 통해 사다리가 제대로 출력되지 않는 것을 발견하게 되었습니다. 그리고 이번 리팩터링 과정에서 진행한 모든 리뷰에서 질문드렸던 검증 위치에 대한 고민을 많이 해보았습니다. 그럼 이번에도 리뷰 잘 부탁드리겠습니다 🙂 |
안녕하세요 춘식🎶! 조이입니다.
기능 구현 방법에 대해 고민하고, 리팩터링 과정에서 어려움이 많아 2번째 PR이 늦어졌습니다.
부족함이 많지만 2단계 미션도 리뷰 부탁드리겠습니다!
미션 관련 내용, 질문은 하단에 작성하였습니다.
변경 사항
User(사용자)에게Position(위치) 값을 추가로 부여하여 결과 산출질문 사항
이번 리뷰도 잘 부탁드리겠습니다:)
그럼 오늘도 좋은 하루 되세요! 🫧🍀