[RIPL] 정민지 자동차 경주 미션 제출합니다.#200
Conversation
|
안녕하세요 민지님 ! 이번 리뷰어를 맡게 된 최서영이라고 합니다ㅎㅎ 이번 미션을 진행하시면서 고민하신 부분이나 중점적으로 리뷰받고 싶은 부분이 있다면 편하게 남겨주세요! 잘 부탁드립니다 😊 |
1. 예외처리의 경우말씀해주신 것 외에도 다양한 예외가 있을 것 같은데요! 지금 단계에서는 어떤 조건이 필요한가를 정답처럼 정하기보다는, 민지님이 생각하는 객체란 무엇인지, 그리고 해당 객체가 지켜야 할 최소한의 상태가 무엇인지를 기준으로 예외를 정해보면 좋을 것 같습니다ㅎㅎ 제가 생각을 해봤을 때 자동차 이름이라면 자동차 객체가 정상적으로 존재하기 위해 필요한 조건이므로
등등을 제한으로 두고 객체를 생성할 것 같습니다! 물론 마지막 조건처럼 영어/한글만 허용할지는 요구사항에 없는 추가 정책이기 때문에, 민지님이 어떤 기준으로 정할지 고민해보셔도 좋을 것 같아요! 그리고 말씀해주신 “값이 비어있는 경우”에 대한 예외는 크게 세 가지 방식으로 나눠볼 수 있을 것 같습니다
세 방식 모두 가능하지만, 어떤 쪽에서 처리할지에 대해 자신만의 기준을 세워보는 게 중요할 것 같아요. 민지님은 세 가지 방법 중 어떤 방식을 선택하고 싶으신가요? |
2. 테스트 대상 및 범위에 대한 고민테스트에 대한 질문을 주셨는데요! 다양한 테스트가 존재하지만 저번 주차의 학습 목표가 단위 테스트였으니 단위 테스트에 대해서 먼저 고민해보면 좋을 것 같아요 이 기준이 명확해지면 어떤 테스트를 작성해야 할지도 조금 더 판단하기 쉬워질 것 같습니다! 3. 패키지 구조에 대한 고민파일 구조와 책임 분리에 대해 고민해주셨는데요!
이때 책임을 어디까지 볼지는 절대적인 정답이 있다기보다는 객체의 역할을 어떻게 정의하느냐에 따라 달라질 것 같습니다! 다만 객체를 정할 때 기준이 아예 없는 것은 아니고 여러 가지 기준이 있습니다. 판단하기 좋은 기준 중 하나는 “이 객체가 어떤 이유로 변경되는가?”일 것 같은데요! 예를 들어 RacingGame이 아래와 같은 이유들로 변경될 수 있다면, 이 변경 이유들이 모두 RacingGame의 책임으로 묶이는 게 자연스러운지 고민해볼 수 있을 것 같습니다.
이 변경 이유들이 모두 “레이싱 게임을 진행한다”는 하나의 책임 안에 있다고 본다면 현재 구조도 가능할 수 있어요! 그리고 NumberGenerator, RandomNumberGenerator, WinnerFinder가 domain 패키지에 있는지도 같은 기준으로 생각해볼 수 있을 것 같습니다. 민지님은 domain 패키지를 어떤 역할을 하는 곳으로 보고 계신가요? 그리고 현재 domain/view 외에 다른 패키지 구조를 만든다면 어떤 기준으로 나눠볼 수 있을까요? |
|
|
||
| public interface NumberGenerator { | ||
| int getNumber(); | ||
| } No newline at end of file |
There was a problem hiding this comment.
파일 마지막에 EOF 처리가 되어 있지 않은 것 같습니다!
코드 작성 후 포매팅이 잘 적용되었는지도 함께 확인해보시면 좋을 것 같아요
| return location; | ||
| } | ||
|
|
||
| public void moveCar(int randomNumber) { |
There was a problem hiding this comment.
moveCar()의 매개변수로 들어오는 값에 대한 검증도 고려해볼 수 있을 것 같아요!
현재 RandomNumberGenerator에서 0~9 범위를 보장하고 있지만, Car 입장에서는 외부에서 어떤 값이든 전달받을 수 있는 구조로 보입니다! 따라서 moveCar(-1)이나 moveCar(10)처럼 요구사항을 벗어난 값이 들어왔을 때 예외를 발생시키면 조금 더 안전한 객체로 만들 수 있을 것 같아요!
There was a problem hiding this comment.
그리고 Car 객체에서 이름과 위치를 관리하는 역할 뿐만 아니라 규칙을 기억하는 역할까지 하고 있네요 ~ !
MOVABLE_NUMBER보다 크거나 같아야 위치를 이동시킨다는 규칙을 Car 객체에서 알고 있다면 "이동 규칙"이 변경되는 경우 어떻게 될까요 ? 지금 규모에서는 신경쓰지 않아도 되지만, 한 번 생각해보는 것도 좋을 것 같아요
|
|
||
| private void validateCarNameLength(String name) { | ||
| if (name.length() > MAX_NAME_LENGTH) { | ||
| throw new IllegalArgumentException("Car name must be 5 characters or less."); |
There was a problem hiding this comment.
혹시 이 예외 메시지를 받는 클라이언트를 어떤 사용자라고 생각하고 작성하셨나요?
현재 입력 안내 메시지는 한국어로 제공되고 있는데, 예외 메시지는 영어로 작성되어 있어 사용자가 에러 상황을 이해하기 어려울 수도 있을 것 같습니다!
클라이언트가 콘솔 프로그램을 사용하는 사용자라면, 에러 메시지도 같은 언어와 톤으로 맞춰주는 것을 고려해보시면 좋을 것 같아요
| } | ||
| } | ||
|
|
||
| public List<Car> getCars() { |
There was a problem hiding this comment.
List<Car>처럼 내부 객체 컬렉션을 그대로 반환하면 어떤 문제가 발생할 수 있을까요? 👀
현재는 cars 필드가 그대로 외부에 노출되고 있어서, 외부 코드에서 리스트를 수정할 수 있는 여지가 생깁니다. 그러면 RacingGame이 관리해야 하는 자동차 목록의 상태가 객체 밖에서 변경될 수 있을 것 같아요.
이를 막기 위해 내부 컬렉션을 그대로 반환하지 않고, 불변 리스트로 감싸서 반환하거나 필요한 값만 전달하는 방식도 고려해보면 좋을 것 같습니다!
| .toList(); | ||
| } | ||
|
|
||
| public RacingGame(String carNames) { |
There was a problem hiding this comment.
접근 제어자와 메서드 배치 순서에 대해서도 한 번 고민해보면 좋을 것 같습니다.
일반적으로 클래스 내부에서는 상수/필드 → 생성자 → public 메서드 → private 메서드 순서로 배치하는 경우가 많습니다!
현재는 private createCars()가 생성자보다 위에 있어서, 객체가 어떻게 생성되는지 흐름을 읽기 전에 내부 구현 메서드를 먼저 보게 되는 구조인 것 같아요.
정해진 정답이 있는 것은 아니지만 클래스의 주요 사용 흐름이 잘 드러나도록 메서드 배치 기준을 세워보면 좋을 것 같습니다 ~
There was a problem hiding this comment.
감사합니다! 이 순서에 대해서는 고민하다가 제 나름대로 실행흐름 순서로 배치를 했었습니다. 확실히 말씀해주신 것 처럼 상수/필드 → 생성자 → public 메서드 → private 메서드 순서로 배치하는 것이 더 읽기 좋을 거 같습니다!
| @Override | ||
| public int getNumber() { | ||
| int number = random.nextInt(MAX_BOUND); | ||
| validateNumber(number); |
There was a problem hiding this comment.
random.nextInt(MAX_BOUND)는 0 이상 MAX_BOUND 미만의 값을 반환하기 때문에, 현재 구현에서는 validateNumber가 실패할 일이 거의 없어 보입니다!
이 검증은 주입받은 Random이 예상과 다른 값을 반환하는 경우까지 방어하기 위한 의도일까요? 👀
Random의 동작을 신뢰한다면 중복 검증으로 볼 수도 있어서, 이 검증이 필요한 이유를 한 번 고민해보면 좋을 것 같습니다
There was a problem hiding this comment.
이 부분에 관해서는 test 코드와 예외처리에 집중하다 보니까 생긴 부분 같습니다.
제가 RandomNumberGeneratorTest에서 랜덤값을 0 미만일 경우와 9 초과일 경우를 강제로 반환하는 테스트를 학습 목적으로 작성해봤었는데 이 과정에서 이 로직이 실제 로직에서도 있어야 한다고 무의식적으로 생각하여 validateNumber를 실제 코드에도 넣은 거 같습니다. 솔직하게 말하면 방어로직까지 테스트해 볼 생각은 아니었습니다. 반영과정에서는 삭제하겠습니다!
| } | ||
|
|
||
| public List<String> findWinners() { | ||
| return new WinnerFinder().findWinners(cars); |
There was a problem hiding this comment.
현재 WinnerFinder는 관리하고 있는 상태가 없는 것 같은데, 객체를 생성한 뒤 메서드를 호출하도록 작성하신 의도가 있으신가요??
상태를 가지지 않는 유틸성 동작이라면 static 메서드로 제공하는 방법도 생각해볼 수 있을 것 같습니다!
추가로 static이 어떤 의미를 가지는지, 그리고 객체를 생성해서 사용하는 방식과 비교했을 때 어떤 장단점이 있는지도 함께 정리해보시면 좋을 것 같아요
| } | ||
|
|
||
| @Test | ||
| @DisplayName("레이싱 여러 번 반복시 캐싱 버그 테스트") |
There was a problem hiding this comment.
이 메서드는 테스트를 돌렸을 때 당연히 정상적으로 동작해야 하는 상황인데 어떤 버그를 테스트하신 건지 궁금합니다
그리고 민지님이 생각하는 캐싱은 무엇이며, 이 메서드에서 캐싱이 어떤 의미로 쓰인 것인지도 궁금하네요
| assertThat(racingGame.findWinners()).containsExactly("white", "red"); | ||
| } | ||
|
|
||
| private static class FixedNumberGenerator implements NumberGenerator { |
There was a problem hiding this comment.
아마 중점적으로 리뷰받고 싶다고 남겨주신 부분이 이 코드이신 것 같네요 !
저라면 우선 동작이 바뀌는 경계값을 가장 중요하게 테스트할 것 같아요! 0부터 9까지 모든 값을 테스트하는 것도 틀린 방식은 아니지만, 값의 범위가 작기 때문에 가능한 선택에 가까워 보입니다. 만약 범위가 0 ~ 9가 아니라 0~1000 또는 그 이상으로 커진다면 모든 값을 테스트하는 방식이 계속 적절할지도 함께 고민해보면 좋을 것 같습니다!
그리고 랜덤값에 직접 의존하면 테스트 결과가 실행할 때마다 달라질 수 있으므로, 고정된 값을 반환하는 테스트용 객체를 사용해서 원하는 상황을 만들고 검증하는 방식은 많이 사용됩니다!
다만, 이때 중요한 것은 “무엇을 테스트하기 위해 랜덤을 고정했는가?”라고 생각합니다.
예를 들어 FixedNumberGenerator를 사용했다면 랜덤 생성 자체를 테스트하는 것이 아니라, 특정 숫자가 주어졌을 때 RacingGame이나 Car가 기대한 대로 동작하는지를 테스트하기 위한 것으로 볼 수 있을 것 같습니다!
There was a problem hiding this comment.
2번 파일 구조에 대한 부분은 앞에서 함께 이야기 나눈 내용이 있어, 해당 답변을 참고해주시면 좋을 것 같습니다!
3번 테스트 코드 작성 형식에 대해서는, @DisplayName을 사용해 테스트 의도를 표현하려고 하신 점은 좋아 보입니다.
형식 자체는 팀이나 프로젝트마다 다를 수 있어서 정답이 하나로 정해져 있지는 않다고 생각합니다. 다만 중요한 기준은 “다른 사람이 테스트 이름만 봐도 어떤 상황에서 어떤 결과를 검증하는지 이해할 수 있는가?”인 것 같아요 ㅎㅎ
그래서 @DisplayName을 작성할 때는 아래 내용을 담아보면 좋을 것 같습니다.
- 어떤 상황에서
- 어떤 동작을 했을 때
- 어떤 결과를 기대하는지
이 기준으로 작성하면 테스트명과 @DisplayName 모두 더 명확해질 것 같습니다!!
한번 생각을 해보았을때 3번의 경우에는 정확도가 높아야 하는, 무결성이 중요한 부분에 활용하면 좋겠다는 생각이 제일 먼저 들었습니다. 일반적으로 돈 같은 금전적인 재산을 다루는 경우, 은행이나 결제 시스템에서 활용하면 좋을 거 같습니다! 아마 저는 1번 방식을 주로 활용하지 않을까 싶습니다! 개인 프로젝트 같은 경우에는 규모가 크지 않고 서버 리소스를 줄이는데도 효과적이니까요. 대신 단체 프로젝트면 값을 가진 객체에도 검증 로직을 넣어둘 거 같습니다. |
2. 단위 테스트저는 이번에는 메서드나 함수를 기준으로 생각했던 거 같습니다. 그 하나의 기능이 제대로 작동하는지 보는 것을 단위 테스트의 목표라고 생각합니다. 3. 파일구조domain을 파일 구조에서 생각해볼 때 기능 정도로 인식하고 있는거 같습니다. 그래서 보통 프로젝트를 한다면 domain형 구조로 많이 코딩을 했었습니다. 그런데 아무래도 규모가 작다보니 오히려 기능 별로 나누는 것이 당장은 코드를 복잡하게 만들 수 있다는 생각에 이번에는 딱히 세분화하지 않았었습니다. |
2 participants
안녕하세요 리뷰이님! 이번에 초록스터디에 참여하게 된 정민지입니다!!
Java를 접한지는 1년 정도가 되었는데 제대로 배워본 건 반 년 정도 되었습니다.
테스트 코드를 작성해 본 건 처음이라고 봐도 무방한 거 같습니다.
이번 미션에서 어려웠던 부분
이번 미션에서는 예외 처리의 범위와 테스트 코드 작성 기준을 정하는 부분이 가장 어려웠습니다.
처음에는 요구사항에 명시된 자동차 이름 5자 이하 조건을 예외 처리했고, 추가로 랜덤값이 0~9 범위를 벗어나는 경우도 예외 처리해보았습니다. 다만 이후에 어떤 예외 상황을 더 고려해야 하는지 판단하기가 어려웠습니다. 예를 들어 자동차 이름이 비어 있는 경우, 자동차 목록이 비어 있는 경우처럼 실제로 막아야 하는 입력인지, 아니면 현재 객체의 책임 밖으로 봐야 하는지 고민되었습니다.
또한 처음에는
main메서드와view를 통해 직접 실행하면서 동작을 확인했는데, 오타나 흐름상의 실수로 오류가 발생했습니다. 이후 이런 부분을 테스트 코드로 검증해보려 했지만, 테스트를 어떤 단위로 나누어야 하는지, 어디까지 테스트하는 것이 적절한지 헷갈렸습니다. 예를 들어 레이싱을 여러 번 진행했을 때 위치가 누적되는지, 자동차 리스트가 비어 있을 때 우승자 조회가 어떻게 동작해야 하는지 같은 테스트가 단위 테스트로 적절한지 확신이 없었습니다.파일 구조도 고민이 되었습니다. 현재는
domain과view로만 패키지를 나누었는데,RacingGame이 자동차 생성, 경주 진행, 우승자 조회를 함께 담당하고 있어 책임이 명확히 분리된 구조인지 확신이 들지 않았습니다. 또한NumberGenerator,RandomNumberGenerator,WinnerFinder같은 클래스들이 모두domain안에 위치하는 것이 적절한지도 고민되었습니다.아직 규모가 작은 미션이라 과하게 패키지를 나누는 것도 좋지 않을 수 있다고 생각했지만, 현재 구조가 책임별로 잘 나뉜 구조인지 리뷰받고 싶습니다.
리뷰에서 중점적으로 봐줬으면 하는 부분
처음 작성해 본 테스트 코드라서 테스트의 단위와 범위가 적절한지 중점적으로 봐주셨으면 합니다.
1. 랜덤값 테스트
자동차 이동 조건을 테스트할 때
@ParameterizedTest를 사용해서 0부터 9까지의 값을 모두 확인하도록 작성했습니다. 실제로도 이렇게 모든 값을 세세하게 검증하는 방식이 좋은지, 아니면 3과 4처럼 동작이 바뀌는 경계값 중심으로 테스트하는 것이 더 적절한지 궁금합니다.또한 랜덤 요소가 있는 코드는 직접 랜덤값에 의존하지 않고, 고정된 값을 반환하는 테스트용 객체를 만들어 검증해보았습니다. 이런 방식으로 랜덤 요소를 분리해서 테스트하는 접근이 적절한지도 리뷰받고 싶습니다.
2. 파일 구조
파일 구조 측면에서는 현재처럼
domain과view로만 나누는 것이 적절한지, 아니면 게임 진행을 담당하는 클래스나 랜덤값 생성 관련 클래스를 별도 패키지로 분리하는 것이 더 나은지도 봐주셨으면 합니다.3. 테스트 코드 작성 형식
@DisplayName을 사용해 테스트 의도를 표현해보았는데, 테스트명이나DisplayName을 작성할 때 권장되는 형식이나 기준이 있는지도 궁금합니다.