[1단계 - 사다리 생성] 조조(조은별) 미션 제출합니다.

[eun-byeol]

소중한 첫 번째 리뷰 감사합니다 😊 여러 질문을 주셔서 나름대로 고민하여 답을 해보았습니다.

1번째 리뷰 후, 주요 수정 사항

프로덕션 코드에 인터페이스를 추가하지 않는 방식으로 리팩터링

• Ladder 정적 팩토리 메서드 시그니처에서 BridgesGenerator 제거
• 대신, RandomBinaryNumbers.pickNumbers() util 정의하여 사용

추가 수정 사항과 질문사항은 답글로 남겨놓았습니다

---

안녕하세요! 조조입니다🐥
페어 프로그래밍을 통해 '사다리 타기' 미션을 진행했습니다.
저는 24시 언제든 slack으로 응답이 가능합니다. 반대로, 피케이의 시간규칙(?)을 알려주시면 좋을 것 같아요!(ex. 몇 시 이후에는 dm 자제 등)

🥔 본인의 상황(feat. TMI)

독창적이고 신박한(?) 코드 보다는, 누구나 예측가능한 코드를 짜는 것을 목표하고 있습니다.
그러나, 스스로 판단 기준이 명확하지 않다 보니 코드를 짤 때 여러 의문점들이 생겼었습니다.
정답을 요구하는 것은 아니지만, 현직에 계신 리뷰어님의 견해 또한 들어보고 싶습니다.

대학을 졸업하고 취준생의 신분에서 우테코에 들어왔습니다.
수료 후 취업을 희망하다 보니, 현직에 계신 분들의 이야기를 많이 듣고 싶습니다.

🤔 코드를 작성할 때 신경 쓴 점

TDD 적용

처음 TDD를 진행하다보니, TDD를 어떻게 적용해야 하는 지에 대한 의문이 있었습니다.
테스트 코드 작성 -> 기능 구현 -> 기능 리팩터링 순으로 개발하는 것이 TDD이다라는 기준을 세우고 진행을 했습니다.
커밋 로그를 참고해서 TDD에 대한 피드백을 받고 싶습니다!

💭 질문

1. primitive vs wrapper 사용 기준이 어떻게 되나요?

우선 제 의견은 이렇습니다.
not null인 상황에선 primitive를 사용하는 걸 default로 가져가고, Collection이나 Generic 같은 문법을 사용해야 할 땐 wrapper 클래스로 변환해 사용하는 것이 좋다고 생각했습니다. 왜냐하면, wrapper는 메모리, 속도 측면에서 불리하기 때문입니다.

2. model을 record로 표현해도 될까요?

제 의견은, 표현해도 된다고 생각합니다.
record 클래스를 통상적으론 dto에서 사용하지만, model에서 사용하지 않을 이유는 없는 것 같습니다.

3. static을 언제 붙이는 것이 좋을까요? controller에도 static을 붙여도 될까요?

객체로 생성할 필요가 없고, 필드를 저장할 필요가 없을 때 static을 붙이는 것이 좋다고 생각합니다. 메서드 호출 시간이 짧아져 효율이 좋기 때문입니다.
그렇다면, controller에서도 같은 이유로 static을 붙이는 것이 좋은 설계일지에 대한 의문이 생깁니다.
저는 static으로 인해 문제가 되지 않은 상황에서는 붙여도 된다고 생각했습니다.

[pkeugine]

안녕하세요 조조!
테스트, 구현해주신 결과물 둘 다 정말 재밌게 리뷰 할 수 있었어요.
다양한 테스트가 있고 OutputView 쪽도 상수화를 잘 해주셔서 바꿔가며 돌리는 재미가 있었습니다 😂

누구나 예측가능한 코드를 짜는 것을 목표하고 있습니다.

앗 이걸 리뷰 코멘트를 다 남기고 마지막 한 마디 남기면서 봤네요 ㅠ
이번 미션동안 그러면 이 목표를 염두에 두고 리뷰하겠습니다 👍

현직에 계신 리뷰어님의 견해 또한 들어보고 싶습니다.

제 견해를 억누르면서 리뷰할 때도 많은데, 앞으로 맘껏 다 말해볼게요 :)

현직에 계신 분들의 이야기를 많이 듣고 싶습니다.

주변에 다양한 사람들에게 물어보고 그들의 생각도 남겨보겠습니다 👍

크게 바꿀점은 많이 없어보여서 자잘한 리뷰 남겨봤어요.
반영하다가 궁금한게 있으시면 24시간 언제든! DM 주세요!

[pkeugine]

Suggested change

	implementation 'com.github.woowacourse-projects:mission-utils:1.1.0'
	implementation ('com.github.woowacourse-projects:mission-utils:1.1.0')

이미 존재하던 implementation 방식과 통일시켜보는건 어떨까요?
사소하지만 첫 시작부터 사소한 것을 잘 지켜줘야 프로젝트가 커져도 통일성이 유지되더라구요 :)

[eun-byeol]

이 부분까지 고려하지 못 했었네요!
build.gradle 파일도 형식 잘 맞춰주도록 하겠습니다👍

[pkeugine]

Q) primitive vs wrapper 사용 기준이 어떻게 되나요?

답변을 하려고 했는데, 조조가 생각해보신 내용이 제 생각과 완전 동일해요.
별개로 어떤 값이 default 로 null 이어야 할까? 또는 숫자 값은 어떨 때 default 로 0 이 아니면 좋을까? 를 고민해보죠!

[eun-byeol]

어려운 질문이었습니다..! 나름대로 정리를 해보았는데, 피케이의 의견도 궁금합니다!!
질문을 숫자값이 null이면 좋은 상황 vs 숫자값이 0이면 좋은 상황으로 해석해서 생각해봤습니다.

숫자값이 null이면 좋은 상황

• 0이 의미 있는 값일 경우
  • 예) 0, 1, 2, 3이 각각 어떤 상태를 표현하고 있을 때, 초기값을 0으로 표현하면 0은 두 가지 상태를 의미하게 되므로 부적절하다.
• 할당되지 않았음(값이 존재하지 않음)을 나타내는 경우
  • 예) DB 테이블에서, Memember의 age 칼럼이 있을 때 사용자 입력을 받지 않은 경우 0보다는 null이 입력되는 것이 자연스럽다.

숫자값이 0이면 좋은 상황

• 0이 의미 없는 값일 경우
  • 초기 상태를 나타내는 0이 다른 상태를 의미를 중복으로 내포하고 있지 않는 경우 적절하다.
• 0이 기준값이 되는 경우(0에서 값이 증가되거나 감소되는 경우)
  • 예) 자동차가 움직인 거리를 0으로 초기화하고, 움직였을 때 거리를 1씩 더해줄 수 있다.

[pkeugine]

저도 같은 생각이에요! 뭔가 추가하고 싶지만 너무 잘 정리해주셔서 달리 추가할 내용이 없네요 👍

[pkeugine]

해당 메서드를 분리하신 이유가 있을까요?
메서드 분리를 통해서 가독성을 챙길 수 있지만, Ladder.create() 라는 표현으로도 충분히 사다리를 만든다는 뜻을 나타낼 수 있을 것 같거든요!

[eun-byeol]

결론 : makeLadder와 Ladder.create 의미가 중복되니 오히려 가독성이 좋지 않은 것 같습니다. 빠르게 리팩터링 했습니다!

메서드를 분리했던 이유?
컨트롤러의 run() 메서드 내부를 입력, 로직실행, 출력 3 단계로 나누고 싶었습니다.
한 번 더 메서드로 감싸서(로직을 구분해서) 사용하는 것이 형식을 맞추고, 가독성을 향상시킨다고 생각했습니다.
그러나, 말씀해주신대로 두 메서드가 완전히 동일하게 읽히기 때문에, 빼는 것이 나은 것 같습니다.
감사합니다!!

[pkeugine]

가독성이 좋지 않다고 생각하진 않았어요 ㅋㅋㅋ 충분히 잘 읽혀요.
그저 같은 내용을 다른 표현을 써서 메서드로 추출할 필요가 있을까 하는 생각이었습니다.

조조가 적어주신 분리 이유에 대한 제 생각 :
세 단계로 나누어도 괜찮다고 생각해요.
복잡한 로직이 포함되어 있다면 메서드로 추출을 통해 이름을 붙일 수 있으니까 코드를 읽을 때 훨씬 쉽게 그 흐름을 파악할 수 있겠죠.
그런데 LadderGameController 의 run 메서드의 경우 입력, 사다리 생성, 출력 이라는 비교적 단순한 로직을 가지고 있어요.
그래서 공백만 적절히 넣어줘도 충분히 단계별로 행위를 나눌 수 있을 것 같아요 :

    public void run() {
        Players players = retryOnException(this::preparePlayers);
        LadderHeight ladderHeight = retryOnException(this::prepareLadderHeight);

        Ladder ladder = Ladder.create(ladderHeight, players, new RandomBridgesGenerator());
        
        OutputView.printGameResultIntro();
        OutputView.printPlayerNames(players);
        OutputView.printLadder(ladder);
    }

[pkeugine]

Bridge 라는 enum 은 연결 되었다, 연결되지 않았다 이 두 값만 가지고 있어요.
이는 boolean 인 true, false 만으로도 충분히 나타낼 수 있는 정보라 해당 enum 이 과한 구현처럼 보여요.
미션 요구사항이 enum 을 사용하라고 했으니 어딘가에는 만들어야하긴 하네요...
나중에 step2 를 진행해보면서 이 Bridge 를 어떻게 활용해볼지 고민해보죠!

이 클래스를 지우라는 말은 절대 아니에요!

[eun-byeol]

nextstep에서 제공하는 랜덤 메서드를 사용하기 위해 boolean 대신 숫자 타입을 선택했습니다.
(boolean을 뽑는 메서드는 없습니다)
다리가 연결되어있다 = 1, 연결되지 않았다 = 0이 충분한 의미를 가진다 생각하지만,
숫자값에 대한 의미를 더욱 부여하기 위해 enum이 합리적이라고 생각했습니다.

추가 Q. 다리 연결 여부가 boolean이 아닌, int형이라면 enum은 합리적인가요?

다리 연결 여부가 boolean 타입이라면 enum을 쓰지 않는 것에 동의합니다. int 라면? 피케이의 의견이 궁금합니다💭

[pkeugine]

아 Randoms 클래스를 사용했기 때문에 0, 1 을 사용했던거군요.
그렇다면 RandomBridgesGenerator 가 Bridge 를 생성하는 방식이 Bridge 쪽에 영향을 주고 있네요.

Q. 다리 연결 여부가 boolean이 아닌, int형이라면 enum은 합리적인가요?

아뇨, 이유를 들어보니 저에겐 더더욱 합리적이지 않습니다
BridgesGenerator 라는 인터페이스를 만들어서 여러 구현체가 생길 수도 있는 점을 고려했죠! 그런데 그 구현체 중 하나가 숫자를 이용한다는 이유 때문에 다른 도메인까지 숫자 정보를 알 필요는 없다고 생각해요.

다시 말해 RandomBridgesGenerator 가 Randoms 를 이용해서 무작위성 로직을 구현한다는 점, 그 로직이 숫자를 사용한다는 점은 RandomBridgesGenerator 외의 다른 도메인 or 클래스가 전혀 알 필요 없는 내용이에요.

nextstep 에서 제공하는 랜덤 메서드를 사용하고자 한 점은 좋아요. 👍
그런데 boolean 을 뽑는 메서드가 없어서 연결유무를 숫자로 나타낸다는건 구현체에서 사용하는 라이브러리 때문에 전체 로직에서 연결유무를 숫자로 나타내야한다는 제약을 두는거에요. 조조가 선택한 방식을 구현체가 따라야하는건데 말이죠!

0 과 1 에 의미를 부여하고 싶다면 지금까지 잘 해오셨던 상수화를 통해 깔끔하게 할 수 있지 않나 싶어요.
RandomBridgesGenerator 에는 이미 0, 1 을 의미하는 START_INCLUSIVE 와 END_INCLUSIVE 가 있죠?
제 개인적인 생각으로 이 클래스에서는 시작 숫자, 마지막 숫자인지 알려주기보단 그 숫자가 로직에서 무엇을 의미하는지 나타내면 좋을 것 같네요. 예를 들어 이걸 UNCONNECTED 와 CONNECTED 로 두면 enum 쪽에 0, 1 정보를 넣지 않고 RandomBridgesGenerator 에서만 숫자로 연결유무를 다룰 수 있어요.

그리고 이렇게 하면 Randoms 사용여부나 그걸로 어떻게 무작위성 로직을 구현했는지와는 관계 없이 다리의 연결 유무를 enum 으로 관리하던지, boolean 으로 관리하던지 조조가 선택할 수 있어요. 그러면 enum 방식을 선택하더라도 Bridge 쪽에 findBridgeByCode 와 같은 특정 구현체를 위한 메서드는 사라지겠네요 :)

[eun-byeol]

결론 : 다른 도메인(Line)이 랜덤 로직(RandomBinaryNumbers)을 의존하지 않게 리팩터링했습니다!

변경 사항

• 기존 util 패키지에 있던 RandomBinaryNumbers 로직을 -> 도메인 패지키 하위에 RandomBridgeGenerator로 수정
• 랜덤숫자 반환 타입 List<Integer>에서 List<Bridge>로 수정
• Bridge enum에서 code(0,1) 필드 및 findBridgeByCode 메서드 삭제

--

(저의 사고 과정은 이렇습니다.)

피케이가 해주신 말씀에 매우 공감합니다.

RandomBridgesGenerator 가 Randoms 를 이용해서 무작위성 로직을 구현한다는 점, 그 로직이 숫자를 사용한다는 점은 RandomBridgesGenerator 외의 다른 도메인 or 클래스가 전혀 알 필요 없는 내용

boolean 을 뽑는 메서드가 없어서 연결유무를 숫자로 나타낸다는건 구현체에서 사용하는 라이브러리 때문에 전체 로직에서 연결유무를 숫자로 나타내야한다는 제약을 두는거에요.

단, 제가 Line을 생성할 때 List<Integer>를 받았던 이유가 있었습니다.
RandomBinaryNumbers는 util로서 사다리 관련 도메인과는 완전히 구분되길 원했습니다.
그래서 List<Bridge>가 아닌 List<Integer>를 반환했고, CONNECTED/UNCONNECTED와 같은 변수명을 사용하지 않았었습니다.

그러나, 이 방식은 말씀해주신 것 처럼 도메인에서 알 필요 없는 정보를 알아야 하는 문제가 있기 때문에
해당 랜덤 util을 domain 로직으로 옮겼습니다.

그렇다면 여기서 고민이 하나 더 생깁니다. RandomBridgeGenerator를 테스트 해야 할까? 피케이의 의견 궁금합니다!
(RandomBridgeGenerator를 테스트 하는 코드는 없지만, 커버리지 상으로는 커버가 되는 것으로 표시됩니다.)

추가 Q. RandomBridgeGenerator를 테스트 해야 하는 지?

[pkeugine]

제가 이해한 것이 맞다면 범용적으로 사용할 수 있는 유틸 클래스를 만들고자 하신거군요!
존중할 수 있는 이유라고 생각해요. 이것저것 시도해보면서 조조가 생각하는 '그 상황에 더 맞는 구조'를 구현해보는거죠.

살짝 의미가 잘못 전달된것 같은데,
제가 DM으로 알려드렸던 것처럼 이 리뷰는 조조가 RandomBinaryNumbers 라는 클래스를 만들기 전을 기준으로 작성했어요.
그래서 RandomBinaryNumbers 가 List 를 반환한다는건 이 당시 모르고 있었습니다... 😅
ff99a97 커밋에 있는 RandomBridgesGenerator 와 Bridge 를 보고 남긴 리뷰에요.
말해주신대로 util 클래스로 분리한다면 도메인과 완전히 구분되는게 좋다고 저도 생각해요.

Q. RandomBridgeGenerator를 테스트 해야 하는 지?
RandomBridgeGenerator 는 step 2 구현하면서 만드신거죠?
테스트를 해야한다, 하지 않아야한다는 일단 정답이 없어요. 안할 수도 있어요. 이걸 테스트 했을 때 애플리케이션이 버그로부터 자유로워질 것 같고, 유지보수하기 더 편해진다면 하는거라고 생각해요. 테스트의 난이도로 봤을 때 특히 random 한 특성은 테스트하기가 까다롭죠. 그래서 저라면 최대한 무작위적 특성을 가지고 있는 로직을 고립시키고, 내가 테스트 가능한 로직만 철저하게 테스트할 것 같아요. RacingCar 미션에서도 Car 에서 랜덤한 요소를 분리시키고 외부에서 어떤 정보를 넣어줬죠? 그게 숫자가 될 수도 있고, 전략 패턴 방식으로 구현했을 수도 있고, 아니면 Car 가 아무런 값을 받지 않고 그냥 move() 만 호출되면 이동하는걸수도 있구요. 저라면 그런 방향으로 랜덤 로직을 분리해볼 것 같아요. 그게 조조가 새로 만든 RandomBridgeGenerator 라면 서로 생각한게 같은거네요! 그런데 아니라면, 이건 저의 생각이니 참고하여 조조가 한 번 판단하여 진행해보시죠! "테스트 난이도에 비해 그 테스트가 주는 가치가 그리 크지 않다"는 이유로 테스트를 하지 않아도 저는 납득할 것 같아요.

[pkeugine]

@FunctionalInterface 를 사용하셨군요 👍
이 어노테이션을 붙인 이유가 궁금하네요!

[eun-byeol]

테스트 코드에서 Ladder 생성 로직을 검증하기 위해 사용했습니다.

그러나, 아래에서 리뷰해주신 내용처럼
과연 테스트를 위해 프로덕션 코드에 FunctionalInterface 를 추가하는지 좋은가?에 대해 고민점이 생겼습니다.

Ladder에서 인터페이스를 받지 않도록 리팩터링을 했고, 아래 피드백 답글로 정리해뒀습니다😊

[pkeugine]

다양한 테스트 방식을 적용해보시고 계시네요! 👍

[pkeugine]

Suggested change

	@DisplayName("라인 생성 시 다리가 겹치지 않으면 예외가 발생하지 않는다")
	@DisplayName("라인 생성 시 다리가 겹치지 않으면 성공")

예외라는 표현은 실제로 예외가 일어나는 테스트에만 사용해보는건 어떨까요?
테스트가 많아질수록 엄청 많은 테스트 제목을 한 번에 보게 되는데, 그럴 수록 제목이 확실하게 차별되는게 덜 헷갈리더라구요

[eun-byeol]

고려하지 못 했던 부분이었습니다. 바로 반영하겠습니다😊

[pkeugine]

테스트를 정말 꼼꼼하게 작성해주셔서 라인 커버리지가 거의 100% 에 달해요! 🥳
intelliJ 에서 테스트를 돌릴 때 커버리지까지 볼 수 있는 방법을 찾아보고, 어디가 커버되지 않았는지 확인해보는건 어떨까요?
테스트 커버리지 100% 달성 에 대해서는 다양한 의견이 있어서 조조의 기준을 만들어보는 것도 좋을 것 같아요.

[eun-byeol]

결론: Line 클래스를 record 대신, class로 바꾸고 불필요한 재정의 메서드를 지우는 방식으로 수정했습니다.

커버리지를 확인해보니, Line record의 equals, hashCode, toString 재정의 메서드가 커버되지 않았습니다.
사실상 해당 로직은 프로덕션 코드에는 사용되지 않은 코드인데, 해당 메서드 때문에 커버리지가 100%을 달성하지 않았습니다.

domain을 record로 정의했을 때, 커버리지에 불리하게 작용할 수 있다는 점도 단점이 되겠네요!!
커버리지 달성 기준에 대해서는 더 고민해보고 추가 질문 드리겠습니다😊

• 수정 전(record)

• 수정 후(class)

[eun-byeol]

커버리지 100% 달성이 유의미한가?

저는, 커버리지 100% 달성이 유의미하다고 생각합니다.
위 댓글 상황처럼, 커버리지 수치를 통해 불필요한 프로덕션 코드를 찾아낼 수 있어 이점이 있다고 생각합니다.
반대로 검증되지 않은 기능에 대해 테스트 코드를 추가하게 될 수도 있겠죠!

나의 기준

프로덕션 코드를 담고 있는 model에서 커버리지 100% 달성하자

(+ 욕심 - Application에 예외 입력 시 오류 메시지를 출력하는 테스트도 추가하여 controller 커버리지도 높여봤습니다..!ㅎㅎ 하지만 이런 수치만 높이려는 것은 좋지 않은 것 같습니다..!)

[pkeugine]

앗 100% 를 꼭 채우라는 의미는 아니었는데...!
이 코멘트의 목적은 커버리지를 한 번 확인해보고, 조조가 적어주신 것처럼 record 의 사소한 부분까지 검증하는 100% 가 유의미한가 vs 숫자가 주는 의미가 크기 때문에 유의미하다 를 생각해보자는거였어요.
제 코멘트를 다시 읽어보니 마치 채우는게 좋겠다는 뉘앙스긴하네요 😅

그럼에도 100% 달성이 유의미하다고 생각하고 그걸 실천하는 모습이 멋있습니다 👏 👏 👏
불필요한 프로덕션 코드를 찾아낼 수 있다 -> 정말 좋은 포인트입니다. 조조가 테스트를 꼼꼼하게 잘 작성했기 때문에 커버되지 않은 로직은 불필요하다 라는 결론이 나온거기도 하죠. 굉장히 좋은 답변이네요.

추가로 수치만 높이려는 것은 좋지 않은 것 같다 는 생각도 동의합니다. 테스트가 많아질수록 테스트 역시 유지보수의 대상이 되고, 수치만 높이려는 테스트는 유지보수성을 좋게 만들지는 않죠. 👍

[pkeugine]

Q) 커밋 로그를 참고해서 TDD에 대한 피드백을 받고 싶습니다!

커밋 로그를 봤는데 테스트를 한 번에 여러개 만들고 진행하시는군요?
보통 TDD 의 흐름이 다음과 같이 흘러가는 거 같아요 :

1. 통과하지 않는 테스트를 하나 만든다
2. 그 테스트를 어떻게든, 최대한 빨리 통과시킨다
3. 리팩토링한다
4. 다시 1번으로

그래서 저라면 일단

• Player 를 정상적으로 만들 수 있는 상황 테스트 작성 후 로직 구현
• Player 를 만들 수 없는 예외 상황 테스트 작성 후 검증 로직 구현
  이렇게 했을 것 같아요.

그런데 이러면 시간이 좀 오래 걸리긴 하죠... 어느정도 타협점이 필요한 것 같아요 🤔
충분히 잘 진행해주셨어요!!!!

[eun-byeol]

제가 이해한 바가 맞을까요?

Player 를 정상적으로 만들 수 있는 상황 테스트 작성 후 로직 구현

-> validateNameLength 로직을 뺀 Player 클래스 및 생성자 구현

Player 를 만들 수 없는 예외 상황 테스트 작성 후 검증 로직 구현

-> validateNameLength 로직까지 구현

    public Player {
        validateNameLength(name);
    }

    private void validateNameLength(String name) {
        if (name.length() > MAX_NAME_LENGTH) {
            throw new IllegalArgumentException(INVALID_NAME_LENGTH);
        }
    }

추가 Q. TDD를 현업에서도 활발하게 사용하는지?

TDD가 개발 초기에 생산성이 낮다, 개발 속도가 느려진다는 의견을 들어본 적이 있습니다.
현업에서는 TDD를 실제로 적용하는지, 한다면 언제 사용하는지 궁금합니다!

[pkeugine]

네 이해한 바가 맞습니다.

1. Player 객체 생성 테스트

@Test
void createPlayerTest() {
    String name = "pk";
    assertThatCode(() -> new Player(name)).doesNotThrowAnyException();
}

2. Player 객체 생성

public class Player {
    
    private final String name;

    public Player(String name) {
        this.name = name;
    }
}

3. 이름 길이 테스트 추가

@Test
void createPlayer_invalidName_nameLengthOverFive() {
    String name = "eunbyeol";
    assertThatThrownBy(() -> new Person(name)).isInstanceOf(IllegalArgumentException.class);
}

4. 이름 길이 검증 로직 추가
5. parameterizedTest 도입
6. 테스트 추가, 로직 추가... etc

근데 누구나 다 같은 단위로 TDD 를 하진 않을거 같아서 참고용으로 봐주세요.

Q. TDD를 현업에서도 활발하게 사용하는지?

하는 분도 있다고는 들었지만 저를 포함해서 제 주변 개발자분들은 하지 않는걸로 알고 있습니다.
조조가 다른 현업분을 의견도 알고싶다고 하셔서 마침 오늘 우테코 리뷰어 친구들 두 명 만났는데 그 둘도 안하고 있다고 했구요.
이유는... 일단 TDD 하려면 어떤 객체를 생성하고, 어떤 로직이 대략적으로 만들어질 것이고, 그 로직을 객체지향적으로 어디에 넣어야 결합도가 낮아질지 알아야 테스트를 미리 작성하고 개발을 할 수 있어요. 하지만 그런 큰 틀은 구현하는 도중 알게 되는 경우가 많아요. 그래서 유명한 말이 있어요: 이런 상황에서 "TDD 를 하고 싶다면 일단 프로젝트를 다 구현하고, 싹 삭제한다음, TDD 를 하면서 다시 구현해라". 또 다른 이유로는 조조가 말해주신 것처럼 개발 속도가 느려진다는 이유도 있어요. 데드라인이 있기 때문에 일단 프로덕션 코드부터 다 완성하고 테스트 짜는 경우가 많아요. 테스트가 프로덕션 코드보다 훨씬 작성하기 어렵고 오래걸리는 경우도 많거든요.

그래서 저는... 현업에선 TDD 를 한 적이 없긴해요 😅

[pkeugine]

누구나 예측 가능한 코드를 짜는 것이 기준이 된다면, 사용자 이름 과 사다리 길이 이 두 가지를 입력받을 때 generic 과 Supplier 를 쓰는건 조금 과한 것 같아요. retryOnException 을 다른 사람이 사용하게 된다면 supplier 에 무엇을 넣어줘야 하는지 직관적으로 알기가 좀 힘들거든요.

약간의 중복이 있더라도 쉽고 읽기 편하게 구현하는게 좋을까?
vs
중복을 최대한 줄이는 것이 좋을까?

이 고민을 한 번 해보죠!

[eun-byeol]

결론 : 반복을 허용하더라도 Supplier에 더 명확한 값을 넣어주도록 리팩터링했습니다.

    private Players retryPlayersOnException(Supplier<Players> preparePlayers) {
        try {
            return preparePlayers.get();
        } catch (IllegalArgumentException e) {
            OutputView.printExceptionMessage(e.getMessage());
            return retryPlayersOnException(preparePlayers);
        }
    }

    private LadderHeight retryLadderHeightOnException(Supplier<LadderHeight> prepareLadderHeight) {
        try {
            return prepareLadderHeight.get();
        } catch (IllegalArgumentException e) {
            OutputView.printExceptionMessage(e.getMessage());
            return retryLadderHeightOnException(prepareLadderHeight);
        }
    }

약간의 중복이 있더라도 쉽고 읽기 편하게 구현하는게 좋을까? vs 중복을 최대한 줄이는 것이 좋을까?

제 의견은 전자입니다.
가독성이 떨어지지만 코드량이 줄어들어 효율적인 상황을 자주 마주했었습니다. 반복문을 stream으로 바꿨는데 오히려 직관적이지 않은 경우는 반복문 그대로 두는 편입니다. 알고리즘을 공부할 때도 짧은 코드가 오히려 가독성을 해치는 경우를 많이 봤었습니다.

말씀해주신 것 처럼, 제3자가 제 코드를 읽었을 때 한 번에 이해하기 어려울 것 같습니다. 현재 입력이 2번 뿐이니 괜찮지만, 더 많은 입력이 추가된다면 중복 코드는 그만큼 늘어나게 되겠죠. 이때는 retryOnException을 클래스로 빼는 것도 대안이 될 것 같습니다.

이 질문에 대한 피케이의 의견도 궁금합니다💭

[pkeugine]

정말 좋네요! 그리고 저도 중복이 있더라도 읽기 쉽게 구현하는 것을 선호해요.
쉽게 만들지 않는다면 한 달 뒤의 나도 지금의 내 생각을 모를 때가 많거든요 😂

반복문을 stream 으로 바꿨는데 오히려 직관적이지 않은 경우는 반복문 그대로 두는 편입니다
저도 동의합니다. Java 의 다양한 기능을 사용하지 않고도 잘 읽히는 코드를 짜는게 정말 잘 짜는 코드라고 생각해요. 오히려 그게 더 어렵죠 🤔

짧은 코드가 오히려 가독성을 해치는 경우를 많이 봤었습니다
저도 동의합니다. 한 줄에 모든걸 다 넣지 않고 지역 변수를 써서 적절하게 이름을 지어주는 것이 좋더라구요.

Q) 피케이의 의견도 궁금합니다
저라면 retryLadderHeightOnException 메서드 자체를 만들지 않고 그냥 preparePlayers 와 prepareLadderHeight 에 try-catch 문을 넣을 것 같아요. 그리고 나중에 여러 입력을 받게되더라도 각각의 prepare 메서드에 다 넣을 것 같습니다. 되게 간단하게 만드는 것같은 느낌이죠? 😂

그 이유는 run 메서드에서 preparePlayers 를 호출하고 Players 객체를 받을 때, run 쪽에서 재입력을 명시해서 넣어주는게 아니라, preparePlayers 쪽에서 스스로 재입력을 받도록 하고 싶어서입니다. 웬만하면 호출하는 쪽에서 덜 신경쓰고 "해줘" 했을 때 호출당하는 쪽에서 다 해줬으면 좋겠어요. (정말 제 개인적인 의견입니다.) 재시도 로직은 입력 받는 쪽이 갖고 있는게 더 적합한 위치인 것 같거든요.

그런데 입력이 정말 너무 많아진다면...
조조가 처음 리뷰 요청을 보냈을 때의 코드 기준으로 (generic 과 supplier 가 다 있는 retryOnException 기준)
retryOnException 메서드를 안으로 옮길 것 같습니다.

    public void run() {
        Players players = preparePlayers();
        LadderHeight ladderHeight = prepareLadderHeight();
        Ladder ladder = makeLadder(players, ladderHeight);
        end(players, ladder);
    }

    private Players preparePlayers() {
        return retryOnException(() -> {
            List<String> playerNames = InputView.askPlayerNames();
            return Players.create(playerNames);
        });
    }

    private LadderHeight prepareLadderHeight() {
        return retryOnException(() -> {
            int ladderHeight = InputView.askLadderHeight();
            return new LadderHeight(ladderHeight);
        });
    }

[eun-byeol]

결론 : 말씀해주신대로 리팩터링했습니다👍

웬만하면 호출하는 쪽에서 덜 신경쓰고 "해줘" 했을 때 호출당하는 쪽에서 다 해줬으면 좋겠어요.
저도 매우 공감합니다!! 가독성 측면에서도 불필요한 정보를 제거하는 것이 유리하다고 생각합니다.

preparePlayers 쪽에서 스스로 재입력을 받도록
제가 생각해내지 못한 방식인데 훨씬 가독성이 좋은 것 같아요. 감사합니다!!

[pkeugine]

안녕하세요 조조! 리뷰 잘 반영해주셨네요 👍
추가로 남겨주신 질문은 코멘트로 남겨놨어요.
이번 리뷰에서 나올 변경점은 그리 크지 않을 것 같아서 step2 진행하면서 적용하셔도 좋을 것 같아요.

comment 에 추가 질문 또는 제 답변에 대한 조조의 생각을 남겨주시면 저도 계속해서 코멘트 달겠습니다! DM 주셔도 좋구요.

고생하셨습니다! step2 에서 뵙죠 :)

[pkeugine]

클래스에 final 을 붙이셨군요! record 에서 바꿔주면서 final 을 붙이신건가보네요. 혹시 이게 어떤 역할을 하는지 아시나요?

지우라는 뜻 아닙니다! 그저 질문이에요 :)

[eun-byeol]

Q. final 클래스의 역할?
상속을 막는 역할입니다.
중요한 로직을 보호하기 위함이라고 알고 있는데,
내부 모든 로직을 overriding 할 수 없어 조작할 수 없고, final 클래스가 사용되고 있는 로직을 보호할 수 있습니다.

record를 일반 class로 변경하면서 자동으로 붙었던 부분인데, 제가 인지하지 못했었네요!

--

여기서 추가 질문 있습니다!

추가 Q. 처음 프로그램을 설계할 때, 자유도가 있게(확장이 쉽게) 열어둘 것인가 vs 제한된 상황에만 동작하도록 막아둘 것인가
예시) final 키워드로 상속을 막지 않을 것인가 vs final 키워드로 막아둘 것인가

이번 미션 뿐만 아니라, 과거 프로젝트를 하면서도 고민했던 포인트였습니다.
요구사항이 자주 바뀌는 상황에서는 '후자' 상황으로 구현했던 도메인 코드를 자주 수정해야 하는 문제가 있었습니다.
그러나, 확장이 쉽게 열어두는 것은 제3자가 제 의도대로 사용한다는 보장이 없기 때문에 '기준'에 대한 고민이 생기게 되는 것 같습니다.
이에 대한 의견 궁금합니다!

[pkeugine]

Q. 처음 프로그램을 설계할 때, 자유도가 있게(확장이 쉽게) 열어둘 것인가 vs 제한된 상황에만 동작하도록 막아둘 것인가

record 를 쓴다면 알아서 final 이 붙을테니, record 를 사용할거냐 마느냐의 고민도 포함되겠네요.
final 관련 주제에서 저는 변경하기 좋은 구조를 만들어야한다고 생각해서 확장하기 쉽게 열어둘 것 같아요.
만약 붙여야 한다면 모든 도메인을 애초에 다 final 처리를 한 뒤, 상속할 일이 있는 경우에만 그 도메인 한정 final 을 풀 수도 있을거구요.

그런데 과거 프로젝트에서도 고민을 하셨다면 클래스 final 처리가 아닌 다른 걸 말하시는걸까요?
자유도가 있게(확장이 쉽게) 열어둘 것인가 vs 제한된 상황에만 동작하도록 막아둘 것인가 라는 질문이 좀 포괄적이라 어느 상황에서나 사용될 수 있는 기준이 있다기보단, 그 상황에 따라서 결정해야할 것 같긴하네요 🤔
어느 상황이든 말해주시면 같이 얘기해볼 수 있을 것 같아요!

[pkeugine]

Suggested change

	.mapToObj(i -> getBinaryNumber())
	.mapToObj(i -> Randoms.pickNumberInRange(0, 1))

1. 이전에 남긴 코멘트 중에서 Ladder.create() 를 메서드 분리할 필요가 있을까? 를 고민해본 적 있었죠? getBinaryNumber() 역시 보면서 같은 생각이 들었어요.
2. 이미 클래스명이 RandomBinaryNumbers 라서 꼭 0 과 1 을 START_INCLUSIVE 와 END_INCLUSIVE 라고 나타내지 않아도 될 것 같아요. 이 상황에서는 0 과 1 그 자체가 스스로를 너무 잘 설명하고 있거든요. INCLUSIVE 라는 표현을 넣고 싶었다면 그럴 순 있다고는 생각해요 🤔

[pkeugine]

Player 에 있는 상수는 private 으로 닫아두고 BRIDGE_LENGTH 를 Player.MAX_NAME_LENGTH 랑은 별개로 관리하는건 어떨까요?

[eun-byeol]

결론 : Player의 상수는 private으로 닫아두고, OutputView에서 해당 값을 따로 관리하도록 수정했습니다.

원래 의도는 Player.MAX_NAME_LENGTH를 다른 값으로 바꿨을 때, BRIDGE_LENGTH를 바꾸지 않도록 하기 위함이었습니다.

그러나, OutputView를 위해 Player 상수를 public 으로 열어두어야 하는 것이 문제가 되는 것 같아 수정했습니다! 소소(?)하지만 고민했던 부분이었는데 예리하게 짚어주셔서 감사합니다.😊

[pkeugine]

Suggested change

	" pobi honux crong jk",
	" |-----| |-----|\n"
	+ " | |-----| |\n"
	+ " |-----| | |\n"
	+ " | |-----| |\n"
	+ " |-----| |-----|"
	);
	assertThat(output()).contains(
	"실행결과",
	" pobi honux crong jk\n",
	" |-----| |-----|\n",
	" | |-----| |\n",
	" |-----| | |\n",
	" | |-----| |\n",
	" |-----| |-----|"
	);

소소한 팁인데요, 이렇게 보기 좋게 바꿔보는건 어떨까요? :)

[eun-byeol]

말씀해주신 대로 출력문 가독성까지 고려하면 더 좋을 것 같아요🤔
그런데, 사다리 순서를 바꿨을 때도 테스트가 통과되는 문제가 있는 것 같습니당!!

[pkeugine]

아 contains 군요...! containsSubsequence 를 사용하면 우리가 원하는 요소들만 특정해서 그 순서도 검증할 수 있어요 :)

Suggested change

	" pobi honux crong jk",
	" |-----| |-----|\n"
	+ " | |-----| |\n"
	+ " |-----| | |\n"
	+ " | |-----| |\n"
	+ " |-----| |-----|"
	);
	assertThat(output()).containsSubsequence(
	"실행결과",
	" pobi honux crong jk\n",
	" |-----| |-----|\n",
	" | |-----| |\n",
	" |-----| | |\n",
	" | |-----| |\n",
	" |-----| |-----|"
	);

근데 생각해보니 그 사이에 다른 줄이 들어가도 통과가 될 수 있긴하네요. 🤔
이건 이런 메서드도 있다는 느낌으로 봐주세요!

[pkeugine]

String 에서 숫자로 변경 👍