[1단계 - 사다리 생성] 조이(김성연) 미션 제출합니다. #113
Conversation
src/main/java/domain/Height.java
Outdated
| import static utils.ErrorMessage.INVALID_LADDER_HEIGHT_RANGE; | ||
|
|
||
| public class Height { | ||
| public final static int MIN_HEIGHT = 1; |
There was a problem hiding this comment.
상수 선언 방법 - static final vs final
먼저 public static이 정말로 필요한지 고민해볼 필요가 있습니다. public static은 모든 외부에서 가져다 쓸 수 있는 것이기에, 자칫하면 캡슐화가 깨지게 됩니다. 범용성이 크지 않다면 프라이빗하게 빼는 게 좋습니다.
There was a problem hiding this comment.
코드를 살펴보니 public final static 을 선언한 변수들은 에러 처리를 할 때 사용되었는데, 이 경우 throw 하는 곳에서 String.format을 하면 ErrorMessage enum은 해당 변수를 몰라도 에러 메세지를 처리할 수 있게 됩니다!
|
|
||
| @DisplayName("1 이상입니다.") | ||
| @Test | ||
| public void heightRangeTest_1() { |
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @DisplayName("사다리의 높이는 ") | ||
| class HeightTest { |
There was a problem hiding this comment.
TDD를 진행 시 커밋 단위
커밋 단위에 답은 정해져 있지 않습니다만, 이상적인 상황에서는 매 커밋 전후로 항상 모든 테스트가 통과해야 한다고 보면 됩니다. 그래야 어떤 커밋으로 테스트가 실패하는지 오류 파악을 하기 쉽거든요. 하지만 테스트 코드가 많으면(지금은 단위테스트만 작성하고 있지만 추후에 통합 테스트까지 작성하게 되면 테스트 코드양이 어마무시해집니다.) 프로덕션 코드와 테스트 코드를 따로 커밋하기도 합니다.
하지만 지금은 TDD를 공부하고 있으니 테스트+코드를 하나의 커밋으로 두면 TDD로 진행을 한 건지 알기가 어렵습니다. 때문에 당장으로서는 테스트와 프로덕션 코드를 따로 커밋하는 걸 추천드립니다!
src/main/java/domain/Users.java
Outdated
|
|
||
| private void validateFirstUser() { | ||
| if (users.isEmpty()) { | ||
| throw new NullPointerException(NOT_FOUND_USER.getMessage()); |
There was a problem hiding this comment.
예외 처리
에러의 원인을 명시화하는 것도 좋지만, 현재로선 이미 자바에서 정의 해 놓은 IllegalArgumentException 하나만 사용해도 재정의한 메세지로 어떤 Exception 인지 충분히 구별이 가능합니다. 또한 try-catch 를 하는 구간이 없던데, try-catch를 넣어보고 왜 하나로 통일하는 게 좋은지 생각해보면 좋을 것 같습니다!
| @@ -0,0 +1,24 @@ | |||
| package utils; | |||
|
|
|||
| public enum LadderFormat { | |||
There was a problem hiding this comment.
enum의 사용
지금은 toString이나 getter를 써도 상관 없지만 저는 getter를 추천합니다. 아래와 같이 쓸 경우 enum화 해야할 상수가 많아지면 중복되는 코드가 많아져 가독성이 떨어집니다. 또한 getter를 사용한다는 건 enum 필드에 값을 저장하는 건데, 이 경우 나중에 enum에서 특정 값을 찾아야 할 때 유용하게 쓰일 수도 있습니다.
Java Enum 활용기를 읽어보면 enum을 언제 어떻게 사용하면 좋을지 자세한 얘기가 적혀있으니, 읽어보면 본인이 이해하기 쉽게 한번 정리해보면 좋을 것 같습니다!
src/main/java/domain/Users.java
Outdated
| private final int FIRST = 0; | ||
|
|
||
| private final List<User> users; |
There was a problem hiding this comment.
필드 변수는 static인 것과 아닌 걸로 분리해주는 편이라서요. 이 경우엔 줄바꿈 없이 붙여주면 좋겠습니닷!
|
|
||
| private List<User> generateUsers(List<String> userNames) { | ||
| return userNames.stream() | ||
| .map(username -> new User(new Name(username))) |
There was a problem hiding this comment.
| .map(username -> new User(new Name(username))) | |
| .map(Name::new) | |
| .map(User::new) | |
src/main/java/domain/Name.java
Outdated
| private void validateNameFormat(String name) { | ||
| if (name.contains(BLANK)) { | ||
| throw new IllegalArgumentException(INVALID_USER_NAME_FORMAT.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
맞춤법에 맞춰 쉼표 뒤에 공백을 넣는 경우를 고려하여 양옆의 공백을 지우는 trim()를 사용해주면 좋을 것 같습니다 :)
| @@ -0,0 +1,24 @@ | |||
| package utils; | |||
|
|
|||
| public enum LadderFormat { | |||
There was a problem hiding this comment.
LadderFormat의 경우 domain과 utils 중에 고민하다가 단순히 상수의 역할을 한다고 느껴 utils로 사용하였습니다.
그런데 다시 리팩터링 하는 과정에서 outputView에서 point 값에 따라 connection status를 반환하는 getConnectionStatus() 를 LadderFormat으로 이동시켰습니다. 코드를 수정하고 다시 고민해보니 utils 보다 domain에 위치하는게 맞는 것 같아서 수정하였습니다.
혹시 괜찮으시다면 이 부분에 대해서도 춘식의 의견을 알 수 있을까요? :)
There was a problem hiding this comment.
현재 LadderFormat은 도메인이면서 View의 정보를 담고 있습니다. (아마 그렇기 때문에 utils에 뒀을 거라 추측되네요.) View에 관한 정보를 없애려면 어떻게 해야할까요?
LADDER_COLUMN을 주목해봅시다. 이 열거형 상수는 OutputView에서만 쓰이고 있습니다. LadderFormat 내부에선 getFormat 말고는 가져올 방법이 없습니다. 그럼 OutputView에만 있어도 되는 것 아닐까요?
LADDER_COLUMN이 없어진다면 이 enum 클래스는 커넥션 정보만 담고 있으므로 LadderFormat 보다는 LadderStatus, ConnectionStatus 등의 이름이 어울릴 것 같네요. 그렇다면 그 둘 또한 View의 정보를 가지고 않아도 되는 방향으로 리팩토링할 수 있을 겁니다.
There was a problem hiding this comment.
의견 감사드립니다 춘식!
다음 미션을 진행하면서 함께 고려해서 리팩터링 해보겠습니다 :)
| import org.junit.jupiter.api.Test; | ||
|
|
||
| @DisplayName("사용자의 이름은 ") | ||
| class NameTest { |
|
안녕하세요 춘식! 소중한 리뷰 감사드립니다. 추가 질문 사항
이번 리뷰도 잘 부탁드리겠습니다:) |
입력값에 대한 검증 방법은 통일시키는게 좋습니다. 전부 InputView에서 검사하거나, InputView에서는 기본적인 입력(null인지, 숫자만 입력했는지 등)만 검증하고 나머진 역할에 따라 검증을 하는 식입니다. 저는 후자를 추천하는데, 전자의 경우 View에서 도메인의 정보를 알게 될 수도 있기 때문입니다.
위치에 관해서라면, 조이 말대로 공통적으로 잡을 수 있는 Main에 두는 게 좋습니다. 그래야 예상치 못한 에러가 났을 때도 잡아줘 돌발적인 시스템 종료를 막기 때문입니다. Main에 두지 않는 경우는, 에러가 발생할만 곳을 예측하여 내가 원하는 에러 모양으로 커스텀하고 싶을 때입니다. (현재 쓰이는 illegalArgumentException이 해당되는 케이스입니다.) 방법이 정확히 무슨 뜻인지는 모르겠지만, 제가 이해한 방향으로 설명 드리자면 에러를 세세하게 잡는 것도 좋지만, 유저한테 에러 정보를 어떻게 전달해야 사용성이 편해질지도 고민해봐야 됩니다. 예시로 이름을 입력해야 할 때
위 3개를 입력 마다 따로 받는 것과
이렇게 하나를 받는 경우의 사용성은 분명히 차이가 있습니다. 너무 적게 줘도 문제지만 너무 많이 줘도 문제가 될 수 있으니 고민해서 적정량의 에러 메세지를 보내는 게 맞다고 생각합니다. :) |
안녕하세요 ~
우테코 백엔드 5기 크루 조이 입니다 :)
(코드를 함께 작성한 페어는
민트입니다)코드를 작성하면서 생긴 질문 사항은 아래 정리해두었습니다.
그럼 리뷰 잘 부탁드리겠습니다!
질문 사항
static finalvsfinal