[1단계 - 사다리 생성] 구름(김민수) 미션 제출합니다. #256
Conversation
| @@ -0,0 +1,47 @@ | |||
| ## 기능 요구사항 | |||
There was a problem hiding this comment.
git 커밋 별 기능 분할이 합리적이어서 리뷰할 때 편했습니다 👍 구름의 기능 개발 과정이 눈에 보이는듯 했네요
| import ladder.view.console.ConsoleInputView; | ||
| import ladder.view.console.ConsoleOutputView; | ||
|
|
||
| public class AppConfig { |
There was a problem hiding this comment.
ㅎㅎ Appconfig에 대해 PR 코멘트에서도 언급해주셨군요!
과하다고 생각할 수 있는데 최대한 확장성을 고려
모든 코드는 트레이드 오프를 감안해야 하는데요, 저는 해당 객체가 주는 확장성 보다는 과하다 쪽이 좀더 신경쓰이더라구요.
이 코드가 어떤 확장성을 가져다주나요? 오히려 import한 객체명이 바뀔때마다 항상 git changed로 잡히는 불편함이 부각되는거 같습니다.
확장성 좋은 코드는 개발자의 꿈이지만, 객체지향 5원칙 SOLID 중 OCP (확장에는 열려있고 변경에는 닫혀있어야한다)에는 Crystal Ball Problem이라는 문제가 있습니다. 우리는 미래의 변경을 수정구를 보는 마법사처럼 모두 알 수 없듯이, 어떤 것이 확장될 지 미리 예측할 수 없다는 의미입니다.
그런 의미에서 목적이 분명치 않은 확장성은 결국 확장성을 누리지 못함으로써 코드의 복잡성을 증대시키고, 코드의 유지보수성을 악화시킵니다.
(과한 확장성을 조롱하는 의미에서 만들어진 FizzBuzz Enterprise Edition이라는 레포지토리도 있습니다 ㅎ 4줄짜리 요구사항을 구현하기 위한 세상에서 제일 복잡한 코드입니다)
개인적인 경험으론 확장성을 고려하지 않고 코드를 모두 구성한 후, 확장의 필요성이 분명한 경우에만 리팩토링하는게 도움이 되었고, 미리 예측했던 확장성이 기대대로 동작했던 경험은 거의 없었던 것 같습니다. (초보여서 그런것도 있을겁니다 ㅎ
말이 길어졌지만 오히려 DI를 지원하는 클래스여서 좋아하는 개발자도 있을겁니다 🫶 다만 저는 확장성이 뛰어난 복잡한 코드보다 바로 이해되는 간단하고 쉬운 코드가 팀에게 더 좋은 코드라고 믿는 개발자 입장에서 의견드렸습니다
| outputView.printLadder(LadderResponse.from(ladder)); | ||
| } | ||
|
|
||
| private <T> T retryOnException(final Supplier<T> supplier) { |
| private final OutputView outputView; | ||
| private final BooleanGenerator booleanGenerator; | ||
|
|
||
| public LadderController(final InputView inputView, |
There was a problem hiding this comment.
ㅎㅎ 모든 인자의 final을 붙이는 어려운 길을 가시는 군요!
항상 가장 강한 제약사항을 걸어두는 것은 좋은 코딩 습관이지만, 언어가 미지원하는 현실속에서 가시밭길이긴 합니다
개인적으로는 현실적으로 유지하기가 쉽지 않아 권하지는 않는 방법입니다
There was a problem hiding this comment.
자동차 미션 피드백에서 "final 키워드를 사용해 값의 변경을 막아라"가 있었어요.
저는 final을 붙일거면 일관되게 다 붙이는게 맞다고 생각해서 다 붙였어요. 고민되네요.
There was a problem hiding this comment.
ㅋㅋ 네 본인 작성 코드에선 노력하시는 게 좋다고 생각합니다~~
나중에 팀단위의 코딩을 하면 이 규칙을 팀 컨벤션으로 설득하는건 어렵지 않은데, 강제할 수 있는 방법이 마땅치 않다보니 (이거 강제하는 기능 만들기는 효과가 별로 없는 작업임) 모든 코드에서 통일되기는 어려운 규칙이라고 생각해요
| import ladder.view.InputView; | ||
| import ladder.view.OutputView; | ||
|
|
||
| public class LadderController { |
| BooleanGenerator booleanGenerator = new RandomBooleanGenerator(); | ||
| Ladder ladder = new Ladder(playerCount, ladderHeight, booleanGenerator); | ||
|
|
||
| assertThat(ladder).extracting("playerCount") |
There was a problem hiding this comment.
바로 위 리뷰와 연결해서, extracting 메서드와 같이 리플렉션으로 접근해서 String으로 된 필드명을 긁는 테스트 방식은 컴파일 오류를 발생시키지 않는다는 점에서 public api 테스트보다 안전하지 않습니다. 가능한 상황이면 api를 통해 테스트를 구성하는 게 좋습니다
There was a problem hiding this comment.
페어와 TDD를 하면서 최대한 getter를 만들지 않기로 해서 extracting을 사용했습니다.
웨지는 TDD를 위해 getter를 다 만드는 편인가요? 아니면 생성자 테스트나 필드 테스트는 하지 않는 편인가요?
There was a problem hiding this comment.
테스트를 위한 프로덕션 코드는 자제하는 편입니다만~~
이런 경우에선 getter를 그냥 뚫어버리긴 해요 ㅋ
getter로 인한 도메인 오염 가능성보다 테스트 잘 되는 이득이 더 크다고 생각해서요
| void testGenerateLadder() { | ||
| int playerCount = 4; | ||
| LadderHeight ladderHeight = new LadderHeight(3); | ||
| List<Boolean> rungExist = List.of(true, false, true, false, true, false, true, false, false); |
There was a problem hiding this comment.
저라면 아래처럼 했을거 같아요!
| List<Boolean> rungExist = List.of(true, false, true, false, true, false, true, false, false); | |
| List<Boolean> rungExist = List.of( | |
| true, false, true, // EXIST, EMPTY, EXIST | |
| false, true, false, // EMPTY, EXIST, EMPTY | |
| true, false, false // EXIST, EMPTY, EMPTY | |
| ); |
테스트는 남이 본다는 시각으로 가능한 친절하게 짜는게 좋습니다. 나만 유지보수 가능한 테스트는 제가 휴가 갔다오고나면 삭제되어 있습니다 (결제 장애가 나서 당장 배포해야되는데 의미를 모르겠는 테스트가 배포를 막고 있다는 상상을 해보시면 좋습니다)
( 요 코드가 나만 유지보수 가능한 수준 이라는 것은 아닙니다~~! 가능한 친절하게 가 리뷰의 핵심입니다 )
| @Test | ||
| @DisplayName("연속되면 발판을 생성하지 않는다. - 1") | ||
| void invalidGenerateRungsV1() { | ||
| List<Boolean> rungExist = List.of(true, true, true); | ||
| Line line = new Line(4, new MockBooleanGenerator(rungExist)); | ||
| List<Rung> rungs = line.getRungs(); | ||
| assertThat(rungs).containsExactly(Rung.EXIST, Rung.EMPTY, Rung.EXIST); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("연속되면 발판을 생성하지 않는다. - 2") | ||
| void invalidGenerateRungsV2() { | ||
| List<Boolean> rungExist = List.of(true, true, false); | ||
| Line line = new Line(4, new MockBooleanGenerator(rungExist)); | ||
| List<Rung> rungs = line.getRungs(); | ||
| assertThat(rungs).containsExactly(Rung.EXIST, Rung.EMPTY, Rung.EMPTY); | ||
| } |
There was a problem hiding this comment.
이 2개의 메서드들은 사실 동일한 내용을 표현하고 있으므로, parameterized 테스트로 표현할 수 있었을거 같아요!
There was a problem hiding this comment.
List.of(true, true)로 테스트를 간소화하는 방향으로 변경했습니다.
| } | ||
|
|
||
| @Nested | ||
| @DisplayName("라인 내 연속된 발판들은 생성되지 않는다.") |
There was a problem hiding this comment.
개인적으로 전 Nested를 쓸 땐 계층 단위로 내려가면서 읽히도록 DisplayName을 적습니다
| @DisplayName("라인 내 연속된 발판들은 생성되지 않는다.") | |
| @DisplayName("라인 내 발판들은") |
이렇게 해두는 편입니다
There was a problem hiding this comment.
좋은 것 같아요!. 다른 부분들도 수정했습니다.
| @ParameterizedTest | ||
| @EnumSource(Rung.class) | ||
| @DisplayName("발판은 상태를 가진다.") | ||
| void testRungStatus(Rung rung) { | ||
| Rung[] values = Rung.values(); | ||
| assertThat(values).contains(rung); | ||
| } |
There was a problem hiding this comment.
초반부 커밋부터 있었던 테스트인데요, 처음엔 Rung 테스트 클래스 만들어 놓는 용도 정도로만 적당한 테스트라고 생각해요
내용이 java enum의 values() 메서드를 테스트하는 거라 학습테스트가 아니었다면 Rung의 isExist나 of 팩토리 메서드를 테스트 해주셨으면 어떨까 생각합니다
There was a problem hiding this comment.
놓쳤던 부분이네요. 수정했습니다.
혹시 values로 테스트는 할 필요가 있는걸까요? 아니면 TDD 초반에만 작성하고 후에 지우는 편인가요?
There was a problem hiding this comment.
ㅎㅎ values 자체가 enum 스펙이니까 자바 메서드를 테스트하는 코드가 되니까 필요없다고 생각해요~
학습테스트로 values 메서드의 동작을 확인해보고 싶었다면 남길수도 있지만요
|
처음에 확장과 재사용을 생각해서 코드를 복잡하게 작성했습니다. 처음 코드에서 config, request dto, exception, 재시도 코드, interface 등등을 제거했습니다. 재리뷰 부탁드립니다. 😭😭😭 위에 피드백에 대한 답변은 전체 리팩토링 전에 답변이었습니다. 죄송합니다. |
|
네네 현업에선 사실 리소스를 그만큼 할당받기가 어려우니까, 리뷰를 받고 수정하는 방향을 권장합니다 👍 (기능상 변경이 없는데 "코드 마음에 안 들어서 다시 짤게요" 는 내 과제가 널널한 상황에서만 가능합니다 ㅎ_ㅎ 내일까지 나가야되는 과제가 n개일 때는 그런 여유를 가지기 어렵죠, 제 시간도 회사와 계약된 리소스입니다) 또 리뷰어의 리소스를 함께 소모하는 거니까요, 너무 자주 반복되면 팀원의 신뢰를 조금씩 잃게 될 수 있다는 점도 고려해주세용 ㅎ 저는 좋습니다~~ 내용 확인하고 리뷰 드릴게요 |
| @@ -1,24 +1,27 @@ | |||
| package ladder.domain.ladder; | |||
|
|
|||
| import java.util.Objects; | |||
| import ladder.exception.ErrorMessage; | |||
| import ladder.exception.InvalidInputException; | |||
|
|
|||
| public class LadderHeight { | |||
There was a problem hiding this comment.
인텔리제이가 record로 변환할 수 있다는 알림을 주네요 ㅎ
누가봐도 코틀린의 data class에 자극 받아서 생긴거 같은 record는 원시값 맵핑 객체와 참 어울리는 개념입니다. (불변이며 보일러플레이트 코드가 다 구현되어있음) 레코드로 구현해보셔도 좋을거 같아요.
There was a problem hiding this comment.
저는 record를 보통 dto에서만 써요!
dto가 아닌 곳에 사용하면 아래와 같이 class를 사용할 경우 getName이고 record를 사용하면 name이잖아요.
뭔가 getAAA가 아닌 점이 일관성이 없는 것 같아서 dto를 제외하고는 class를 선호해요!
웨지는 dto가 아닌 다른 곳들에서도 쓰는 편인가요? 궁금해요!
public record Car(String name) {}
Car.name // 사용class Car {
public String getName() {}
}
Car.getName() // 사용There was a problem hiding this comment.
네 저는 vo에서도 써용 ㅎ
말씀하신 것처럼 get 이 안 붙는 컨벤션이 다른 자바 빈들이랑 달라서 불편할 때는 있어요 (가끔 getXXX 로 자동완성 기다리는데 아무것도 안 뜨는 ㅋ)
근데 보일러플레이트 코드 (equals, hashcode 등) 신경 안 써도 되는게 넘 편해요
| if (height < MINIMUM_HEIGHT) { | ||
| throw new InvalidInputException(ErrorMessage.INVALID_LADDER_HEIGHT_RANGE); | ||
| throw new IllegalArgumentException(String.format("사다리의 높이는 %d이상이어야 합니다.", MINIMUM_HEIGHT)); |
| boolean previousRungExist = index > 0 && rungs.get(index - 1).isExist(); | ||
| boolean canMakeRung = booleanGenerator.generate(); | ||
| private Rung generateRung(List<Rung> rungs) { | ||
| boolean previousRungExist = !rungs.isEmpty() && rungs.get(rungs.size() - 1).isExist(); |
There was a problem hiding this comment.
rungs.get(rungs.size() - 1) 부분을 getPreviousRung() 등으로 메서드 분리해주면 더 잘 읽힐거 같습니다.
1이 매직넘버기도 한데요 중요하진 않은거 같네요
There was a problem hiding this comment.
어제의 저였다면 모든 매직 넘버를 상수화했을텐데요.
오늘의 저는 매직 넘버에 대해서 다시 한 번 생각볼려고 해요.
매직 넘버를 왜 하는가?
저는 가독성을 위해 값에 이름이 필요한 경우나 재사용이 되는 곳이 있을 경우에만 상수화하기로 했어요!
| boolean previousRungExist = index > 0 && rungs.get(index - 1).isExist(); | ||
| boolean canMakeRung = booleanGenerator.generate(); | ||
| private Rung generateRung(List<Rung> rungs) { | ||
| boolean previousRungExist = !rungs.isEmpty() && rungs.get(rungs.size() - 1).isExist(); |
There was a problem hiding this comment.
!rungs.isEmpty() 이건 그냥 rungs의 사이즈를 1 이상만 가능하도록 제약하여 이 클래스에선 신경 안써도 되도록 하는 것도 좋아보여요.
There was a problem hiding this comment.
아래와 같이 수정하라는 말씀이실까요??
boolean previousRungExist = rungs.size() > 0 && rungs.get(rungs.size() - 1).isExist();generateRungs에서 빈 리스트를 만들고 사이즈가 0이면 rungs.get(rungs.size() - 1).isExist()이 부분에서 인덱스 에러가 발생해서 작성해줬어요.
public List<Rung> generateRungs(int playerCount) {
List<Rung> rungs = new ArrayList<>();
for (int i = 0; i < playerCount - 1; i++) {
Rung rung = generateRung(rungs);
rungs.add(rung);
}
return rungs;
}There was a problem hiding this comment.
아니오 ㅎㅎ
아예 도메인 객체의 불변식(항상 지켜져야하는 validation)으로 항상 객체가 한 개 이상이 들어있도록 하는 방법은 어떤가 생각해봤어요~
(이 객체는 setter가 없으니 생성자 validation으로 추가되겠죠 / playerCount가 0 일 수 없다)
이러면 도메인 로직에서 계속 신경써야하는 isEmpty 검사가 없어져도 되니까요
There was a problem hiding this comment.
음? generateRungs가 생성자에서 사용돼요.
List<Rung> rungs = new ArrayList();를 만들고 rung을 하나씩 만들어서 담는데 처음에는 비어있으니까 rungs.get(rungs.size() - 1).isExist(); 이 부분에서 인덱스 에러가 발생해요.
그거를 막기위해서 rungs.size > 0이나 !rungs.isEmpty()를 사용했어요.
다른 부분을 말씀하시는건가요?? ㅠㅠ
|
|
||
| return Rung.of(!previousRungExist && canMakeRung); | ||
| if (previousRungExist || !currentRung.isExist()) { |
There was a problem hiding this comment.
부정연산자가 가독성을 떨어뜨린다는 의견이 있어서 currentRung.notExist() 를 만들어주는 방법도 있습니다
There was a problem hiding this comment.
그렇군요!
메서드가 늘어나는 것이 안좋다고 생각했었는데 isEmpty()같은 것을 사용하면 가독성이 높아지겠군요!
There was a problem hiding this comment.
아하 죄송해요ㅜ suggestion으로 했어야 의도가 정확했겠네요
| if (previousRungExist || !currentRung.isExist()) { | |
| if (previousRungExist || currentRung.notExist()) { |
위 처럼 하는게 어떤가 하는 의견이었어요.
물론 검사식 자체를 메서드로 추출하는 것도 좋습니다
| if (previousRungExist || !currentRung.isExist()) { | |
| if (isExist()) { |
public 메서드가 늘어나는건 안 좋아요. 객체 역할이 많아지고 있다는 신호이고, 역할이 많다 == 여러사람이 동시에 수정할 일이 많아진다 == 컨플릭트가 늘어난다 입니다. 또 역할이 많으니 클라이언트도 많아지고, 나중엔 손도 못대는 코드가 됩니다 ㅎ ㅎ (모든 클라이언트의 정상 동작을 보장해줘야 하니까 겁나서 수정을 못 함)
근데 private 개수는 딱히 상관 없는거 같아요
There was a problem hiding this comment.
아 이해했어요. 제가 위에 적었던 isEmpty()가 Rung Enum에 있는 메서드를 말하는거였어요.
package ladder.domain.ladder;
public enum Rung {
EXIST,
EMPTY;
public static Rung of(boolean exist) {
if (exist) {
return EXIST;
}
return EMPTY;
}
public boolean isExist() {
return this == EXIST;
}
public boolean isEmpty() {
return this == EMPTY;
}
}| } | ||
|
|
||
| public void run() { | ||
| final Players players = retryOnException(this::readPlayers); | ||
| final LadderHeight ladderHeight = retryOnException(this::readLadderHeight); | ||
| Players players = readPlayers(); |
| outputView.printLadderResultMessage(); | ||
| outputView.printPlayerNames(PlayersResponse.from(players)); | ||
| outputView.printLadder(LadderResponse.from(ladder)); | ||
| } | ||
|
|
||
| private <T> T retryOnException(final Supplier<T> supplier) { |
There was a problem hiding this comment.
이 부분이 사라진건 좀 아쉽네요
기존엔 요청이 실패해도 재시도가 가능한 로직이었는데, 리팩토링 이후엔 기능이 사라졌습니다
리팩토링의 가장 중요한 요소 중 하나는 "원래 하던 동작을 그대로 수행할 수 있을 것"입니다!
There was a problem hiding this comment.
최대한 저를 내려놓고 기본적인 요구사항과 간결한 코드로 작성하고 싶었어요.
step2에서는 다시 작성하겠습니다. 😅
There was a problem hiding this comment.
네넹~ 메서드 명이 분명하니 그럭저럭 읽히긴 하는데 사실 Funtional 인터페이스를 메서드 시그니처에 활용하면 주의할 게 "정보가 적어진다"는 점이 있습니다. (Funtion 같은게 인자, 응답 타입이면 띠용 하게 됨)
게다가 요 코드는 타입핑 제약없는 T 메서드 제네릭이니까 더 헷갈립니다 (사용 코드 1~2개는 봐야 좀 감이 옴)
또 자바가 함수형 프로그래밍보다는 객체지향에 더 특화되어있기도 하고, 함수형에 익숙하지 않은 개발자가 많아서 팀원들의 이해도를 고려해가며 써야합니다 ㅎ
저는 스트림 같이 함수형을 좀 지원하는 라이브러리랑 호환시킬 목적의 메서드나, 템플릿 메서드 패턴(특정 구간의 동작만 구현체마다 다른 인터페이스)으로 추상화 할 일이 있을 때만 써요
There was a problem hiding this comment.
재입력 받는 부분이 많아서 아래의 코드가 반복적으로 사용되어 변경하기가 고민되네요.
while (true) {
try {
// 재입력 받는 부분에 대한 코드
} catch (IllegalArgumentException e) {
outputView.printErrorMessage(e.getMessage());
}
}
|
step2를 진행하면서 생겼던 질문입니다. dto에 특별한 비즈니스 로직이나 검증 코드가 들어가지 않는데 테스트할 필요가 있는가 궁금합니다. |
안녕하세요 웨지! 뽀로로와 함께 페어 프로그래밍한 구름 ☁️ 입니다!
이번 미션의 주제는 TDD로 최대한 테스트 먼저 작성한 후 구현할려고 노력했어요.
코드가 복잡해 보일 수 있는데, 최대한 확장성을 고려해서 만들어봤어요.
제 프로젝트 구조는 다음과 같아요.
⭐️ AppConfig
InputView, OutputView, BooleanGenerator가 interface이고 LadderController에서 interface로 받기 때문에 AppConfig에서 의존성 주입을 할 수 있도록 만들어줬어요. 과하다고 생각할 수 있는데 최대한 확장성을 고려해서 해봤어요.
⭐️ View
view와 controller 사이에서 값을 항상 dto로 전달받아요. to나 from 메서드를 통해서 객체에서 또는 객체로 mapping을 해줘요.
converter나 mapper를 만들어서 분리할 수도 있다고 생각하는데, 복잡도가 지나치게 높아져서 사용하지 않았어요.
⭐️ Exception
에러 메세지를 enum으로 관리했고, BaseException을 만들고 그것을 상속받은 Exception들을 만들어서 사용했어요.
⭐️ Domain
players, player
이름을 가지는 참가자들을 player이라 하고 players라는 일급 컬렉션을 만들어서 사용했어요.
name을 원시 값 포장할 수 있다고는 생각했는데 과하다고 생각했고, 변경에 비용이 크지 않아서 하지 않았어요.
ladder
ladderHeight에 검증 로직이 들어가서 원시 값 포장해서 사용했어요.
이것을 원시 값 포장하지 않고 LadderHeightRequest라는 dto에서 간단한 검증을 해도 될 것 같기도 해요.
Ladder은 사다리의 행인 Line들을 가지고 Line은 Rung을 가져요.
여기서 Rung은 비어있거나, 가로 줄이 존재하거나 를 나타내는 enum예요.
ladder가 생성될 때, line들이 생성되고, 연속되지 않는 rung들이 생성돼요.
⭐️ Test
테스트에서 BooleanGenerator를 구현한 MockBooleanGenerator를 사용했어요.
BooleanGenerator는 generate를 쓰면 랜덤 boolean을 반환해요.
MockBooleanGenerator는 생성자에서 boolean 리스트를 받고, generate를 사용할 때마다 인덱스를 옮겨서 테스트할 수 있게 해줘요.
미리 리뷰 감사드립니다.