[RIPL] 정종진 자동차 경주 미션 제출합니다.#199
Conversation
There was a problem hiding this comment.
안녕하세요, 종진님! 이번 자동차 경주 미션 리뷰를 맡게 된 임혜정입니다.
전반적으로 미션 열심히 구현해주신 흔적이 잘 나타나서 저도 열심히 리뷰했네요~!
저도 잘 부탁드립니다 🐰
[이번 미션에서 어려웠던 부분]
- 예외 처리 범위 & 2. 테스트를 위한 코드
리뷰하면서 답변이 되었습니다 😁
- 랜덤 요소의 테스트
랜덤 자체를 테스트해야 한다기보다는 이 랜덤이 사용되는 값을 고정시켜볼 수 있을 것 같아요.
어떤 방법들이 있을까요?
[리뷰에서 중점적으로 봐줬으면 하는 부분]
- 예외 처리 범위와 방식 & 2. 테스트만을 위한 코드
리뷰하면서 답변이 되었습니다 😁
- 어디까지 테스트할지 — private vs public
저는 보통 private 메서드를 직접 테스트하기보다는, 그 메서드를 사용하는 public 메서드의 결과를 통해 함께 검증하는 편이에요. private 메서드는 외부에 공개된 기능이라기보다는, 객체 내부 구현을 나누기 위한 보조 메서드에 가깝다고 생각합니다.
그래서 private 메서드를 직접 테스트하고 싶어진다면, 먼저 두 가지를 고민해보면 좋을 것 같아요!
- 이 로직이 정말 외부에서 독립적으로 검증해야 할 만큼 중요한 책임인가?
- 그렇다면 이 책임이 현재 클래스 안의
private메서드로 숨어 있는 것이 적절한가?
예를 들어 RacingGame의 findMaxPosition()이나 findCarsAt()은 private 메서드지만, 결국 getWinners()를 통해 동작이 드러나고 있어요. 따라서 단독 우승, 공동 우승 같은 케이스를 getWinners()로 검증하면 내부 private 메서드의 동작도 함께 확인할 수 있지 않을까요?
- 랜덤 값 생성 책임 분리
제가 보기에는 현재 Car가 랜덤 값을 직접 해석하고 이동 조건까지 알고 있기 때문에 테스트 상황을 만들기 어려워진 것으로 보여요.
테스트에서 제어하고 싶은 건 랜덤 숫자가 아니라 각 자동차가 몇 칸 이동하는가에 가까우니까요!
제가 남긴 Car.move(int random) 코멘트와도 이어지는 부분이라, 한번 생각해보고 공유해주세요~!
| public void move(int random) { | ||
| if (random < 4) { | ||
| return; | ||
| } | ||
| position++; | ||
| } |
There was a problem hiding this comment.
move(int random)은 전달받은 숫자가 4 미만이면 움직이지 않고, 4 이상이면 위치를 증가시키고 있네요~
Car가 자동차의 위치를 이동시킨다는 책임 뿐만 아니라 어떤 숫자일 때 이동할 수 있는가라는 이동 조건도 함께 알고 있는 구조로 보여요!
만약 이동 조건이 4 이상이면 이동에서 짝수면 이동한다는 조건으로 바뀐다면 어떻게 될까요? 🤔
제가 보기엔 그때마다 Car의 move 메서드가 계속 변경될 가능성이 있어 보여요.
이런 문제는 어떻게 해결할 수 있을까요?
There was a problem hiding this comment.
가장 먼저 떠오른 생각은 하나의 메서드가 하나의 책임만 가지도록,
이동하는 역할과 이동 조건을 검증하는 역할을 분리하는 것이었습니다.
public void move(int number) {
if (isMovable(number)) {
position++;
}
}
private boolean isMovable(int number) {
return number >= 4;
}이렇게 작성하면 move는 움직이는 역할만 하고, 이동 조건이 바뀌더라도
isMovable만 수정하면 되니 move는 그대로 둘 수 있다고 생각했습니다.
하지만 작성하다 보니, 이게 결국 OCP를 위반하는 건 아닐까 하는 의문이 들었습니다.
그때마다 Car의 move 메서드가 계속 변경될 가능성이 있어 보여요.
리뷰에 남겨주신 것처럼, 이동 조건이 계속 바뀐다면 Car 내부의 isMovable을
그때마다 수정해야 하는 일이 생길 것 같았습니다.
그래서 떠오른 것이 전략 패턴이었습니다.
Random을 분리했던 것처럼 이동 조건도 바깥으로 분리하면, 조건이 바뀌더라도
기존 코드를 수정하는 대신 새로운 구현체를 추가해 주입하는 형태로 대응할 수 있을 것 같았습니다.
public interface MovingStrategy { // 전략
boolean isMovable(int number);
}
// 4 이상이면 이동하는 전략
public class NumberMovingStrategy implements MovingStrategy {
@Override
public boolean isMovable(int number) {
return number >= 4;
}
}
public void move(MovingStrategy strategy, int number) {
if (strategy.isMovable(number)) {
position++;
}
}이렇게 하면 이동 조건이 "짝수일 때 이동" 등으로 바뀌더라도, 새로운 구현체를 만들어
주입해주는 것만으로 대응할 수 있어 SRP뿐 아니라 OCP까지 만족시킬 수 있을 것 같습니다!
지금까지 배운 것을 토대로 접근해봤는데, 혹시 더 좋은 방법이나 놓친 부분이 있을까요?
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public class CarTest { |
There was a problem hiding this comment.
Junit 테스트에서는 테스트 클래스와 메서드에 public을 생략하는 걸 권장하는데요!
그 이유는 무엇일까요? 🤔 학습해보고 저에게 공유해주세요~!
hint : Less is More
There was a problem hiding this comment.
여기서도 두가지 코멘트를 드리고 싶은데요.
1. 혼재된 메서드 네이밍
현재 테스트 메서드 이름을 보면 CarNameTest, CarNameLengthTest, CarMoveTest처럼 대문자로 시작하고 있어요.
Java 메서드 이름은 일반적으로 camelCase를 사용하기 때문에, 테스트 메서드도 carNameTest, carNameLengthTest, carMoveTest처럼 소문자로 시작하는 형태가 더 자연스러워 보입니다!
2. 경계값 테스트케이스
자동차 이름 길이는 5자 이하만 허용된다는 요구사항을 예로 들어볼게요.
경계값은 요구사항이 바뀌거나 조건문이 실수로 변경되었을 때 문제를 잘 잡아주는 지점이라서, 아래와 같은 케이스를 추가해보면 좋을 것 같아요.
- 이름이 정확히 5자인 경우 생성 성공
- 이름이 6자인 경우 예외 발생
이렇게 하면 name.length() > 5 조건이 의도대로 동작하는지 더 명확하게 검증할 수 있을 것 같습니다!
경계값 테스트와 다른 테스트케이스도 풍부하게 챙겨주세요😁
요구사항에 없는 예외까지 방어적으로 처리하는 것이 좋을까요?
이게 만약 실제 프로덕트였다면 저는 클라이언트나 기획자에게 직접 확인할 것 같아요.
다만 지금은 학습 목적의 미션이고, 요구사항이 꽤 심플하게 주어져 있다 보니 명시되지 않은 부분을 스스로 고민해보는 것도 좋은 연습이 될 거라 생각해요. (여러 예외 케이스들을 생각해보는 것도 중요하다고 생각해서요 😃)
말씀해주신대로 자동차 이름이 빈 값이면 어떻게 할지, 시도 횟수가 0이나 음수면 어떻게 할지, 자동차 이름이 중복되면 허용할지 같은 부분들이 있을 수 있겠네요 ~
그런데 제가 종진님이 어떤 기준으로 요구사항을 정의했는지 알 수는 없어서, 먼저 README에 직접 정의한 예외 상황과 처리 기준을 정리해보면 좋겠습니다. README에 이런 식으로 정리해주세요! (이번 미션 뿐 아니라 추후 다른 미션에서도 이 방식을 권장합니다~!)
설계 단계에서 예외를 먼저 정리하고 처리하시는지, 아니면 개발하면서 "여기서 터질 것 같다" 싶은 부분을 그때그때 처리하시는지도 궁금합니다.
저 같은 경우는 설계 단계에서 미리 예외 케이스들을 정의하고 개발을 시작하는 편이에요.
그런데 그래도 놓치는 부분이 있을 수 있어서 ㅋㅋㅋ 그런 건 개발하면서 추가로 보완합니다!
There was a problem hiding this comment.
저는 RacingGame이라는 클래스를 봤을 때, 게임을 진행하고 흐름을 제어하는 클래스로 생각했거든요.
그런데 RacingGame은 게임 진행 뿐만 아니라 자동차 이름 목록을 Car 목록으로 변환하고, 전체 자동차를 이동시키고, 가장 멀리 간 위치를 찾아 우승자를 찾는 책임까지 가지고 있네요! 이 로직들은 모두 자동차 여러 대의 묶음과 관련된 책임으로 볼 수 있을 것 같아요~
그래서 저는 Cars라는 일급 컬렉션을 도입하면 좋겠다는 생각이 들었어요!
일급 컬렉션이란 무엇인지, 왜 사용해야 하는지 학습해보고 저에게 알려주세요 😆
There was a problem hiding this comment.
요 아이는 사실 테스트를 위해 만들어진 클래스처럼 보이네요~
FixedNumber는 랜덤 값을 고정해서 테스트를 쉽게 만들기 위한 용도로 사용되고 있는데요. 실제 애플리케이션 실행 흐름에서는 RandomNumber만 사용되고 있어요. 그렇다면 FixedNumber가 프로덕션 코드인 src/main/java에 있어도 괜찮을까요? 👀
There was a problem hiding this comment.
FixedNumber는 랜덤 값을 고정해 테스트를 쉽게 만들기 위한 용도라,
실제 실행 흐름에서는 쓰이지 않고 테스트에서만 필요한 클래스입니다.
같은 인터페이스의 구현체이다 보니 자연스럽게 src/main/java에 두었는데,
실제 사용 흐름까지는 미처 생각하지 못했던 것 같습니다.
테스트에서만 필요한 클래스인 만큼 src/test/java 하위로 옮기도록 하겠습니다!
| public Car(String name) { | ||
| this(name, 0); | ||
| } |
There was a problem hiding this comment.
이렇게 테스트를 편하게 하려고 프로덕션 코드에 생성자를 추가해도 괜찮은 걸까요?
테스트를 위해 프로덕션에 코드를 추가해야 한다는 건 저는 '뭔가 잘못됐는데...?'라는 신호로 보고요.
일단 테스트하기 어려운 코드란 무엇인지부터 생각해보면 좋을 것 같아요.
그 다음 내 코드에서 왜 테스트가 어려웠는지, 어떻게 해결해볼 수 있을지 한번 방향을 잡아봅시다 😆
근데 이게 생각보다 쉽진 않아서, 해결 방안이 생각나지 않는다면 생각해본 단계까지만 공유해주세요~!
같이 한번 해보죠 ! ㅋㅋㅋ
++) 그런데 테스트 전용 생성자가 꼭 필요했을까요? 👀
2 participants
안녕하세요 혜정님!
이번 초록스터디 스터디원으로 참여하게 된 정종진입니다.
Java를 학습한 지는 약 1년 정도 됐고, 테스트 코드를 직접 작성해보는 건 이번이 거의 처음입니다.
바쁘신 와중에 리뷰해주셔서 감사합니다! 앞으로도 잘 부탁드립니다!!
이번 미션에서 어려웠던 부분
1. 예외 처리 범위
이번 미션에서 명시적으로 걸린 제약은 "자동차 이름은 쉼표(,)로 구분하며 5자 이하만 가능하다" 하나여서, 우선 이 부분에 대해서만 예외 처리를 해두었습니다. 다만 코드를 작성하다 보면 그 외에도 처리할 만한 예외들이 떠올랐는데, 이런 것들까지 함께 처리하는 편이 좋은지 판단이 어려웠습니다.
2. 테스트를 위한 코드
"모든 로직에 단위 테스트를 구현"이라는 요구사항 때문에, 처음에는
private메서드들도 전부public으로 바꿔서 테스트했습니다. 그런데 이렇게 캡슐화를 깨면서까지 내부 로직을 직접 테스트하는 것이 맞는지, 오히려 구조를 망가뜨리는 건 아닌지 기준을 잡기가 애매했습니다.테스트를 편하게 하려고 기존 코드에 손을 대는 경우도 있었는데(테스트 전용 생성자, 접근 제어자 변경 등), 이게 정당한 트레이드오프인지 아니면 테스트를 위해 코드를 오염시키는 것인지 판단이 어려웠습니다.
3. 랜덤 요소의 테스트
자동차의 전진 여부가 랜덤 값에 의존하다 보니, 그대로 두면 "특정 자동차가 우승했는지" 같은 결과를 확정적으로 검증할 수 없었습니다. 이 랜덤 요소를 어떻게 제어 가능한 구조로 분리할지가 고민이었습니다.
리뷰에서 중점적으로 봐주셨으면 하는 부분
1. 예외 처리 범위와 방식
요구사항에 명시된 "이름 5자 이하" 제약만 우선
Car생성자에서 검증하도록 구현했고, 위반 시 예외를 던지도록 했습니다.RuntimeException을 사용했는데,IllegalArgumentException처럼 더 구체적인 타입을 사용하는 것이 더 좋은지 의견을 듣고 싶습니다.이렇게 요구사항에 명시된 예외만 처리하는 현재 범위가 적절한지, 아니면 떠오르는 예외들도 함께 방어하는 편이 좋은지 궁금합니다.
2. 테스트만을 위한 코드
확정적인 테스트를 위해 프로덕션 코드에 두 가지를 추가했습니다.
Car(String name, int position)— 위치를 지정한 자동차를 만들어 우승자 로직을 검증하려고 추가한 테스트 전용 생성자입니다.RacingGame(List<Car> cars, Number number)— 미리 만든 자동차 목록과 숫자 생성기를 직접 주입하는 테스트 전용 생성자입니다.이렇게 테스트를 편하게 하려고 프로덕션 코드에 생성자를 추가해도 괜찮은 걸까요?
Car(String, int)는 위치를 마음대로 정할 수 있다 보니, 실제 경기라면 모든 자동차가 0에서 똑같이 출발해야 하는데 누군가는 앞선 위치에서 출발할 수 있게 됩니다. 테스트엔 편하지만, 이런 생성자가 열려 있다는 것 자체가 실제 게임에서는 치명적인 버그로 이어질 수 있다고 생각합니다. 이런 위험을 감수하고 열어두는 게 맞는지, 아니면 더 안전한 방법이 있는지 궁금합니다.3. 어디까지 테스트할지 — private vs public
처음에는 우승자를 구하는 내부 계산(
findMaxPosition,findCarsAt)도 각각 테스트하려고public으로 바꿨습니다.그런데 이 메서드들은 원래
getWinners()안에서만 쓰는 보조 메서드라, 테스트하겠다고 굳이 밖으로 꺼내는 게 맞나 싶었습니다. 게다가 나중에 우승자 계산 방식을 변경하게 되면, 결과는 똑같은데 이 메서드를 직접 부르던 테스트만 깨질 것 같았습니다.그래서 두 메서드는 다시
private으로 바꾸고, 최종 결과인getWinners()만 테스트하기로 했습니다. 우승자가 기대한 대로 나온다면 그 안의 내부 계산도 제대로 돈 것이니, 내부 메서드는 결과를 통해 자연스럽게 검증된다고 생각했습니다.getWinners())로만 검증하는 방향이 적절한지, 아니면 핵심 계산은 따로 직접 테스트하는 편이 나은지 궁금합니다.4. 랜덤 값 생성 책임 분리
자동차의 전진 여부가 랜덤 값에 의존하다 보니, 랜덤 생성 부분을
Number인터페이스로 분리해RacingGame에 주입하도록 했습니다.RandomNumber를 사용합니다.FixedNumber를 주입해, 전진/멈춤을 원하는 대로 할 수 있게 했습니다.다만 여기엔 한계가 있었습니다.
FixedNumber는 한 라운드에서 모든 자동차에게 같은 값을 주기 때문에, "전부 전진하거나 전부 멈추는 것만" 가능하고 "특정 자동차 한 대만 앞서게" 만들기는 어려웠습니다. 그래서 특정 우승자를 검증하는 테스트를 하려면 다른 방법이 필요했고, 결국 2번에서 말씀드린Car(String name, int position)생성자로 자동차의 시작 위치를 직접 지정하는 방식을 쓰게 되었습니다.