[RIPL] 천성진 자동차 경주 미션 제출합니다.#196
Conversation
|
안녕하세요! 성진님!! 말씀해주신 부분들을 고려하고 중점적으로 봐주시길 요청하신 부분들 위주로 한번 살펴보도록 할게요! |
|
미션에서 어려웠다고 하신 부분들을 너무 잘 구현해주셨어요!! 여러 선택지들을 생각하고 성진님의 근거를 바탕으로 합리적인 선택을 해주신 것 같아요. 테스트 코드에서도 경계값들을 놓치지 않고 케이스 작성해주셨고, 요구사항에 맞는 테스트를 작성해주셔서 테스트 메서드만으로 요구사항을 파악하는 것도 가능했습니다! 고생하셨어요!! ㅎㅎ |
| @@ -0,0 +1,5 @@ | |||
| package domain.generator; | |||
|
|
|||
There was a problem hiding this comment.
난수 생성기를 함수형 인터페이스를 사용해서 일종의 전략 패턴처럼 런타임에 주입하게 해주는 방식을 사용하셨네요!
현재 수준에서 딱 맞는 수준으로 구현하셨다고 생각합니다!
과하지도 않고 덜하지도 않게 알맞은 방식을 선택하신 것 같아요!
전 이런 함수형 인터페이스로 사용하고 싶을 경우
@FunctionalInterface 어노테이션을 사용해서 의도를 명시하는 편이에요.
해당 어노테이션이 하는 역할에 대해서 한번 알아보셔도 좋을 것 같아요
There was a problem hiding this comment.
@FunctionalInterface에 대해 찾아보니, 추상 메서드가 하나인 인터페이스에 사용해서 해당 인터페이스를 함수형 인터페이스로 사용하려는 의도를 명시하는 어노테이션이라는 것을 알게되었습니다!
해당 어노테이션은 인터페이스가 함수형 인터페이스의 조건을 만족하는지 컴파일 시점에 검사해주는 역할을 합니다. 즉, 추상 메서드가 하나만 있어야 한다는 조건이 깨지면 컴파일 에러를 통해 바로 확인할 수 있습니다.
현재 NumberGenerator는 generate()라는 하나의 동작만을 가지는 인터페이스이고 RacingGame에서 숫자 생성 방식을 외부에서 주입받기 위한 역할을 하고 있습니다.
특히 테스트 코드에서는 () -> 4처럼 람다식을 사용해 원하는 숫자 생성 전략을 간단히 주입하고 있습니다.
NumberGenerator에 @FunctionalInterface를 붙이면, 이런 람다 기반 사용 방식을 의도한 설계로 드러내고 이후에도 해당 구조가 깨지지 않도록 컴파일 단계에서 확인할 수 있습니다!
| this.position = 0; | ||
| } | ||
|
|
||
| public void move(int number) { |
There was a problem hiding this comment.
현재 RandomNumberGenerator에서는 0부터 9 까지의 값을 생성해 주고 있지만, Car는 외부에서 전달받은 값에 대한 검증 없이 사용하고 있는 것 같네요!
그래서 음수처럼 예상하지 못한 값이 전달되어도 현재는 별다른 문제가 발생하지 않는 것처럼 보입니다.
요구사항에 이 값이 반드시 0 이상이어야 한다는 규칙이 있다면, Car에서 입력값을 검증하고 적절한 예외를 던지도록 하는 것은 어떨까요?
반대로 음수도 단순히 이동하지 않는 값으로 허용하는 정책이라면, 현재 구현도 괜찮을 것 같습니다.
Car가 RandomNumberGenerator의 구체적인 동작 방식에 의존하기보다는, 전달받은 값에 대한 자신의 유효성 규칙을 명확히 갖도록 하면 더 안전할 것 같아요.
There was a problem hiding this comment.
move()에 전달되는 값이 항상 0~9 범위일 것이라고 암묵적으로 기대하고 있었다는 점을 놓쳤던 것 같습니다.
음수처럼 예상하지 못한 값이 전달되어도 현재 구현에서는 단순히 이동하지 않는 것처럼 동작해서, 별다른 문제가 없어 보인다고 생각하고 무심하게 넘어갔던 것 같습니다.
말씀해주신 것처럼 Car는 전달받은 값에 따라 자신의 상태를 변경하는 객체이므로, 전달받은 값에 대한 유효성 규칙도 Car 내부에서 명확히 갖는 것이 더 안전한 것 같습니다!
|
|
||
| @Test | ||
| void 전진_조건이면_주어진_횟수만큼_모든_자동차의_position이_증가한다() { | ||
| RacingGame racingGame = new RacingGame(cars, () -> 4); |
There was a problem hiding this comment.
테스트에서 원하는 값을 반환하도록 람다식을 전달해 실제 난수 생성 동작을 대체하고 있네요!
이런 방식은 테스트 더블의 특정 유형과 유사한 역할을 하는데요.
어떤 유형인지 직접 찾아보시고 알려주세요!
There was a problem hiding this comment.
해당 방식은 테스트 더블 유형 중 Stub과 유사한 역할을 한다고 생각합니다!
처음에는 테스트 더블이라는 용어와 각 유형의 차이가 다소 헷갈렸습니다.
그래서 각 개념을 아래와 같이 정리해보았습니다.
테스트 더블 유형
테스트 더블은 테스트 과정에서 실제 객체를 대신하여 사용하는 객체를 의미
- Dummy: 실제로 사용되지는 않지만, 객체 생성이나 메서드 호출을 위해 자리만 채우는 객체
- Stub: 테스트에 필요한 값을 미리 정해두고 호출 시 그 값을 반환하는 객체
- Fake: 실제 구현처럼 동작하지만, 운영 환경에서 쓰기에는 단순화된 구현체
- Spy: 실제 객체처럼 동작하면서 호출된 내용이나 횟수 등을 기록하는 객체
- Mock: 특정 메서드가 어떤 인자로 몇 번 호출되었는지 등 '행위 검증'을 목적으로 사용하는 객체
RacingGameTest에서는 실제 난수 생성 로직을 사용하는 대신, 항상 4라는 정해진 값을 반환하는 람다식을 전달하고 있습니다.
실제 랜덤 값을 사용하면 테스트 실행마다 결과가 달라질 수 있기 때문에, 테스트에서 원하는 값을 고정해서 랜덤성을 제거한 것입니다.
그렇기 때문에 이 람다식은 테스트에 필요한 값을 미리 제공해서 테스트 상태를 제어하는 Stub의 역할과 유사하다고 생각했습니다.
| } | ||
|
|
||
| public List<Car> getCars() { | ||
| return List.copyOf(cars); |
There was a problem hiding this comment.
List.copyOf(cars)를 사용해 리스트 자체의 변경은 막았지만, 내부의 Car 객체는 동일한 참조를 유지한다는 점까지 고민하셨군요!
말씀하신 CarStatus는 출력 계층에 전달할 DTO 또는 읽기 전용 상태 객체로 볼 수 있을 것 같은데요.
현재 미션 규모에서 Car의 상태 변경 방식이 제한적이라면 DTO를 추가하지 않고 현재 구조를 유지하는 것도 충분히 합리적인 선택이라고 생각합니다.
반대로 도메인 객체인 Car를 직접 노출하지 않으며 외부에서 변경하지 못하게 하는 것을 중요하게 본다면, CarStatus로 이름과 위치만 변환해 반환하는 방식도 고려해볼 수 있겠네요!
어떤 선택이든 각각의 트레이드오프가 있을 것 같아요!
선택한 방식과 선택하지 않은 방식의 장단점을 설명할 수 있다면, 현재 선택도 충분히 좋은 설계라고 생각합니다!
| } | ||
|
|
||
| public List<Car> getCars() { | ||
| return List.copyOf(cars); |
There was a problem hiding this comment.
지금 보니 프로덕션 코드에서의 getCars()의 사용처가 한 곳뿐이네요! 매 라운드가 종료된 후 자동차들의 위치를 View 계층으로 전달하기 위한 용도로 보입니다.
만약 자동차 이름이 중복될 수 없다는 도메인 규칙이 있다면, 자동차 이름을 키로, 위치를 값으로 갖는 Map<String, Integer> 형태로 변환해 반환하는 방식도 고려해보면 어떨까요?
이렇게 하면 View 계층에는 출력에 필요한 정보만 전달할 수 있을 것 같습니다. 하지만 출력 순서가 중요하다면 Map의 순서는 어떻게 보장할 수 있을지 고민해보시면 좋을 것 같네요!
이 방식도 마찬가지로 트레이드오프가 존재할 것 같네요!
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
|
||
| public class CarTest { | ||
|
|
There was a problem hiding this comment.
해당 클래스의 테스트 메서드들은 소괄호,() 다음에 중괄호,{} 사이의 띄어쓰기가 없네요!
| 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.
테스트 클래스에는 public 접근 제어자를 붙이지 않아도 동작하는데, 그 이유가 무엇일까요?
JUnit이 테스트 클래스를 어떤 방식으로 발견하고 실행하는지, 그리고 Java의 기본 접근 제어자가 어떻게 적용되는지 함께 살펴보시면 좋을 것 같아요.
There was a problem hiding this comment.
테스트 클래스에 public을 붙이지 않아도 동작하는 이유
테스트 클래스에 public을 붙이지 않아도 동작하는 이유는 접근 제어자를 생략하면 package-private 접근 수준이 적용되고 JUnit 5가 package-private 테스트 클래스와 메서드를 실행 대상으로 허용하기 때문입니다.
package-private는 같은 패키지 안에서는 접근할 수 있도록 허용하는 접근 수준입니다.
JUnit는 테스트 클래스와 테스트 메서드가 반드시 public일 필요가 없도록 설계되어 있기 때문에, package-private 상태에서도 정상적으로 테스트를 실행할 수 있습니다.
JUnit 4에서는 public 선언이 필수였지만, JUnit 5에서는 테스트를 외부에 공개할 필요가 없다는 설계 철학에 따라
public 강제가 제거되었다는 점도 알게 되었습니다!
JUnit이 테스트를 발견하고 실행하는 과정
IntelliJ에서 테스트 실행 버튼, Gradle의 test 작업을 실행하면 IntelliJ 또는 Gradle이 JUnit Platform을 실행합니다.
전체적인 흐름은 다음과 같습니다.
IntelliJ 또는 Gradle에서 테스트 실행
↓
JUnit Platform 실행
↓
JUnit Jupiter 테스트 엔진 동작
↓
실행 대상으로 전달된 클래스 탐색
↓
리플렉션으로 클래스와 메서드 정보 조사
↓
@Test 등의 어노테이션이 붙은 메서드 발견
↓
테스트 클래스 인스턴스 생성
↓
테스트 메서드를 리플렉션으로 실행
위 흐름을 조금 더 자세히 살펴보면 다음과 같습니다.
실행할 테스트 후보 전달
IntelliJ에서 특정 테스트 클래스의 실행 버튼을 누르면 해당 클래스가 실행 대상으로 JUnit Platform에 전달됩니다.
Gradle의 test 작업을 실행한 경우에는 test 소스 세트와 설정된 필터를 바탕으로 실행할 클래스들이 선택되어 JUnit Platform에 전달됩니다.
이때 프로젝트의 모든 클래스를 전달하는 것이 아니라, src/test/java 아래의 클래스나 *Test, *Tests와 같은 기본 규칙에 해당하는 테스트 후보를 먼저 찾은 뒤, 해당 클래스들만 실행 대상으로 전달합니다.
Jupiter 엔진이 후보 클래스 조사
JUnit Platform은 전달받은 실행 대상 정보를 JUnit Jupiter 엔진에 전달합니다.
Jupiter 엔진은 JUnit 5 테스트를 발견하고 실행하는 역할을 담당합니다. 전달받은 클래스 내부에 실행할 테스트가 있는지 확인하며, 이 과정에서 리플렉션을 사용합니다.
리플렉션으로 테스트 메서드 발견
리플렉션을 사용하면 프로그램 실행 중에 클래스에 선언된 메서드와 어노테이션 정보를 확인할 수 있습니다.
단순화해서 살펴보자면 다음과 같이 클래스의 메서드를 조회하고 @Test가 붙어 있는지 확인합니다.
Class<?> testClass = CarTest.class;
for (Method method : testClass.getDeclaredMethods()) {
if (method.isAnnotationPresent(Test.class)) {
// Jupiter 테스트 메서드로 등록
}
}테스트 객체 생성 및 메서드 실행
테스트 메서드를 발견하면 Jupiter 엔진은 테스트 실행 순서를 관리하고 실제 테스트를 수행합니다.
이 과정에서도 리플렉션을 이용해 테스트 클래스의 객체를 생성하고, @Test가 붙은 테스트 메서드를 호출하여 실행합니다.
| } | ||
|
|
||
| private void validateTryCount(int tryCount) { | ||
| if(tryCount <= 0){ |
There was a problem hiding this comment.
if문에서 키워드와 소괄호 사이, 조건식과 중괄호 사이에 공백이 누락되어 있네요!
이 부분 말고도 for문이나 다른 부분에서도 공백 누락이 종종 보이는데요
IntelliJ의 Java 포맷터를 활용하면 이런 스타일을 일관되게 적용할 수 있을 것 같아요!
There was a problem hiding this comment.
평소에 크게 신경 쓰지 않았던 부분이라 중간중간 많이 놓쳤던 것 같습니다 🥲
컨벤션은 단순히 코드 모양을 맞추는 규칙이 아니라, 다른 사람이 코드를 읽을 때 불필요한 해석 비용을 줄이기 위한 약속이라고 생각합니다. 공백이나 들여쓰기가 일관되지 않으면 코드의 흐름을 파악하기 어려워지고, 리뷰 과정에서도 실제 로직보다 형식에 더 많은 비용이 들 수 있다고 느꼈습니다.
앞으로는 더 신경 쓰겠습니다!
2 participants
안녕하세요 경표님!
이번 초록스터디 리뷰이로 참여하게 된 천성진입니다.
Java를 제대로 접하고 학습한 지는 1년 정도 된 것 같고, 테스트 코드를 작성하는 것은 거의 처음입니다.
그래서 이번 미션을 진행하면서도 단순히 테스트 코드를 작성하는 것보다,
에 대한 기준을 잡는 것이 어렵게 느껴졌습니다.
특히 랜덤 요소가 포함된 로직을 어떻게 테스트 가능한 구조로 분리해야 하는지, 그리고 실행 결과를 어떤 기준으로 검증해야 의미 있는 테스트가 되는지 고민이 많았습니다.
바쁜 시간 내어 리뷰해주셔서 감사합니다. 잘 부탁드립니다!
이번 미션에서 어려웠던 부분
가장 어려웠던 부분은. 랜덤 값을 생성하는 책임을 어디에 둘 것인지였습니다.
처음에는
Car가 직접 랜덤 값을 생성하거나,Car가Random객체를 가지는 구조를 생각했습니다.하지만 그렇게 하면 모든 자동차가 랜덤 생성 객체를 가지게 되어 불필요하다는 생각이 들었습니다.
반대로 랜덤 생성 로직을
static으로 전역처럼 사용하면 구현은 단순해지지만, 테스트에서 랜덤 값을 제어하기 어려워지는 문제가 있었습니다.그래서 최종적으로는
NumberGenerator인터페이스와RandomNumberGenerator구현체를 분리하고,RacingGame에 주입하는 방식으로 구현했습니다.이렇게 하면 테스트에서는 고정된 값을 반환하는 generator를 사용할 수 있고, 실제 실행에서는 랜덤 값을 생성하는 구현체를 사용할 수 있다고 판단했습니다.
다만 이 구조가 현재 미션 규모에 비해 과한 설계는 아닌지 고민이 되었습니다.
단순히 랜덤 값을 생성하는 구현체 하나만 필요한 상황이라면 인터페이스까지 분리하는 것이 과할 수도 있다고 생각했습니다.
하지만
NumberGenerator를 인터페이스로 분리해두면 테스트에서는 고정된 값을 반환하는 구현으로 바꿔 끼울 수 있고, 나중에 랜덤 값 생성 방식이나 범위가 바뀌더라도RacingGame의 변경을 줄일 수 있다는 장점이 있다고 판단했습니다.그래서 테스트 가능성과 변경 가능성을 고려해 현재 구조를 선택했지만,
어느 정도의 변경 가능성까지 열어두는 것이 적절한지에 대한 기준은 아직 어렵게 느껴집니다.
리뷰에서 중점적으로 봐주셨으면 하는 부분
1. 랜덤 값 생성 책임 분리
NumberGenerator인터페이스와RandomNumberGenerator구현체를 두고,RacingGame에서 주입받는 방식이 적절한지 궁금합니다.현재 구조가 테스트 가능성을 높이고, 랜덤 값 생성 방식의 변경에도 대응하기 쉬운 방향이라고 생각했습니다.
그러나 해당 미션 규모에 비해 과하게 추상화한 것은 아닌지도 고민됩니다.
2.
RacingGame.getCars()의 반환 방식현재
RacingGame에서는 자동차 목록을 출력하기 위해cars를 반환하고 있습니다.리스트 자체는
List.copyOf(cars)로 복사해서 반환하고 있지만, 리스트 내부의Car객체는 동일한 참조를 가지고 있기 때문에 외부에서Car객체에 접근하면 원본 객체의 상태도 변경될 수 있다는 문제가 남아 있다고 생각합니다.이 부분에서 고민한 선택지는 다음과 같았습니다.
List<Car>를 반환한다.CarStatus같은 별도 객체를 만들어 이름과 위치만 반환한다.CarStatus같은 객체를 만들면 외부에서 도메인 객체를 직접 변경할 가능성은 줄어들 것 같지만, 현재 미션 규모에서는 코드가 조금 복잡해지는 느낌도 있었습니다.이 부분에서 현재 구현이 괜찮은 선택인지, 아니면 출력용 상태 객체를 분리하는 편이 더 좋은 설계였을지 궁금합니다.