-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
step3) 자동차 경주 게임 구현 #6040
base: smileeeey
Are you sure you want to change the base?
step3) 자동차 경주 게임 구현 #6040
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수민님 안녕하세요!
3단계 미션 잘 진행 해 주셨네요 :)
코멘트를 좀 남겨 뒀는데, 확인 후 리뷰 재요청 해주세요
고생 많으셨습니다 ~
} | ||
|
||
private RacingCar[] getParticipant() { | ||
while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while문을 쓸 필요가 있는걸까요 ?
} | ||
} | ||
|
||
void validateNum(int num) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
접근지정자가 없는 메서드들이 있네요 ~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트코드를 위해 default 접근지정자를 사용했습니다.
private으로 두고 테스트하는 방법들을 찾아보았는데
- 리플렉션 사용하는 것은 캡슐화를 깨트리는 방식이라 제외했고
- 해당 private method를 사용하는 public method를 테스트하는 방식은 메소드별로 단위 테스트를 작성하고자 하는 방향성과 멀어지는 것 같았습니다.
대안으로 default 생성자를 사용하고 테스트코드에서 사용하는 방식을 사용했는데요, 다른 대안이 있다면 말씀해주세요:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트코드를 위해 접근지정자를 변경하는 등의 기존 코드에 영향을 주는건 바람직하지 못합니다!
그리고 기본적으로 테스트코드의 대상은 public이라 private 메서드를 직접 테스트하지는 않으셔도 돼요!
해당 private method를 사용하는 public method를 테스트하는 방식은 메소드별로 단위 테스트를 작성하고자 하는 방향성과 멀어지는 것 같았습니다.
이건 아마 .. 모든 로직에 단위 테스트를 구현한다
라는 요구사항 때문에 말씀하신 것 같은데, 여기서 말하는 모든 로직은 public 메서드를 뜻합니다! 기본적으로는 public 메서드를 통해 내부에서 사용되는 private 메서드가 같이 검증되어야 하고, 만약 이게 어렵거나 private 메서드를 직접 테스트해야 하는 상황이 있다면 그건 클래스 설계가 잘못되지 않았는지 의심해봐야 해요
numberVerifier.verify(num); | ||
} | ||
|
||
RacingCar[] createRacingCars(int count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 그냥 빈 배열을 만들어주는 것 같아요?
추가로, 특별한 이유가 없다면 배열보단 List 등의 Collection을 사용하는 걸 추천드려요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리스트는 배열보다 대략 3배의 메모리를 더 사용한다고 알고 있습니다!
요구사항으로 볼 때 한 번 입력받은 뒤로 참여자가 추가되는 케이스는 없는 것으로 보여 사이즈를 고정해도 문제가 없을 것 같아 배열을 사용했습니다~!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀드리기전에 먼저 .. 리스트가 3배의 메모리를 사용하는 근거가 혹시 뭘까요 ? ArrayList면 기본 배열 사이즈때문에 조금 쓸 수 있어도 사이즈 지정을 해주면 부가정보 조금을 제외하면 크게 차이 없을 것 같고 .. LinkedList면 노드 크기와 타입에 따라 가변적이라 계산이 나오기가 어려울 것 같은데.. 궁금하네요!
말씀주신대로 문제는 없고 배열도 배열의 장점이 있어 필요한 경우가 있는데요. 대부분의 경우 List가 가지는 조작의 편의성과 가독성 그리고 확장성 때문에 배열보다는 List를 사용하는게 효율적이라고 생각해서 배열을 지양하시는게 어떨까 하고 드린 의견이었구요!
이건 대답을 안주셨는데, 44번 라인 빈 배열만 만들고 속이 안채워져있는 것 같아요~ 지금 프로그램 실행하면 NPE가 나지 않나요 ?
|
||
public class InputView { | ||
private static Scanner scanner = new Scanner(System.in); | ||
int requestParticipant() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 접근지정자가 없네요 ~
import java.util.Random; | ||
|
||
public class RacingCar { | ||
private int pos; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변수명에는 줄임말을 지양해주세요 ~
} | ||
} | ||
|
||
protected void incPos() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요게 있는데 setter는 왜 있는걸까요 ?
그리고 이건 왜 protected인가요 ?
|
||
@BeforeEach | ||
void setUp() { | ||
InputView mockInputView = mock(InputView.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mocking 한 객체를 아래에서 사용하지 않는것으로 보이는데요, mocking 한 이유가 있을까요 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별다른 이유는 없어서 실제 객체를 사용하도록 변경했습니다~
@Test | ||
void testValidateNames_validNames() { | ||
int TEST_COUNT = 3; | ||
assertDoesNotThrow(() -> carRacingGame.validateNum(TEST_COUNT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트는 기본적으로 public 메서드에 대해서만 진행하고, 테스트를 위해 접근지정자 레벨을 조정하는 건 지양해야 합니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 말씀드렸듯이.. 접근 지정자 조정보다 나은 해결책을 잘 모르겠습니다..
@Test | ||
void makeRandomNum() { | ||
int random = car.makeRandomNum(); | ||
assertTrue(random >= 0 && random <= 9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 현재는 무조건 통과하는 테스트이고, 혹시 범위가 0~20 정도로 바뀌게 되면 이 테스트는 성공하기도, 실패하기도 하는 테스트라 검증을 못하고 넘어갈 수도 있어서 적절치 못한 테스트에요
랜덤한 값을 생성하고 그 값이 4 이상인 경우 true
를 리턴해주는걸 하나의 전략으로 정의하고 추상화해서 RacingCar가 인터페이스를 통해 주입받을 수 있게 하면 mocking을 통해 해당 인터페이스의 메서드가 불린지만 확인하는걸로 테스트를 만들어 볼 수 있을 것 같아요!
@Test | ||
void makeRandomNum() { | ||
int random = car.makeRandomNum(); | ||
assertTrue(random >= 0 && random <= 9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 현재는 무조건 통과하는 테스트이고, 혹시 범위가 0~20 정도로 바뀌게 되면 이 테스트는 성공하기도, 실패하기도 하는 테스트라 검증을 못하고 넘어갈 수도 있어서 적절치 못한 테스트에요
랜덤한 값을 생성하고 그 값이 4 이상인 경우 true
를 리턴해주는걸 하나의 전략으로 정의하고 추상화해서 RacingCar가 인터페이스를 통해 주입받을 수 있게 하면 mocking을 통해 해당 인터페이스의 메서드가 불린지만 확인하는걸로 테스트를 만들어 볼 수 있을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 감사합니다~ 리뷰 주신 내용 반영하고 일부는 제 방식대로 해석해서 답변 남겨두었습니다! 부족한 점이 있다면 말씀부탁드립니다 :)
} | ||
} | ||
|
||
void validateNum(int num) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트코드를 위해 default 접근지정자를 사용했습니다.
private으로 두고 테스트하는 방법들을 찾아보았는데
- 리플렉션 사용하는 것은 캡슐화를 깨트리는 방식이라 제외했고
- 해당 private method를 사용하는 public method를 테스트하는 방식은 메소드별로 단위 테스트를 작성하고자 하는 방향성과 멀어지는 것 같았습니다.
대안으로 default 생성자를 사용하고 테스트코드에서 사용하는 방식을 사용했는데요, 다른 대안이 있다면 말씀해주세요:)
numberVerifier.verify(num); | ||
} | ||
|
||
RacingCar[] createRacingCars(int count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리스트는 배열보다 대략 3배의 메모리를 더 사용한다고 알고 있습니다!
요구사항으로 볼 때 한 번 입력받은 뒤로 참여자가 추가되는 케이스는 없는 것으로 보여 사이즈를 고정해도 문제가 없을 것 같아 배열을 사용했습니다~!
return "RacingCar{" + "pos=" + pos + '}'; | ||
} | ||
|
||
int makeRandomNum() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별도 클래스로 역할 구분했습니다~
|
||
@BeforeEach | ||
void setUp() { | ||
InputView mockInputView = mock(InputView.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
별다른 이유는 없어서 실제 객체를 사용하도록 변경했습니다~
@Test | ||
void testValidateNames_validNames() { | ||
int TEST_COUNT = 3; | ||
assertDoesNotThrow(() -> carRacingGame.validateNum(TEST_COUNT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에서 말씀드렸듯이.. 접근 지정자 조정보다 나은 해결책을 잘 모르겠습니다..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수민님 안녕하세요!
피드백 반영 잘 해 주셨네요!
그런데 프로그램이 수행이 안되고 있어서 한번 더 확인 부탁드릴게요!
코멘트 확인 후 리뷰 재요청 해주세요 ~
numberVerifier.verify(num); | ||
} | ||
|
||
RacingCar[] createRacingCars(int count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀드리기전에 먼저 .. 리스트가 3배의 메모리를 사용하는 근거가 혹시 뭘까요 ? ArrayList면 기본 배열 사이즈때문에 조금 쓸 수 있어도 사이즈 지정을 해주면 부가정보 조금을 제외하면 크게 차이 없을 것 같고 .. LinkedList면 노드 크기와 타입에 따라 가변적이라 계산이 나오기가 어려울 것 같은데.. 궁금하네요!
말씀주신대로 문제는 없고 배열도 배열의 장점이 있어 필요한 경우가 있는데요. 대부분의 경우 List가 가지는 조작의 편의성과 가독성 그리고 확장성 때문에 배열보다는 List를 사용하는게 효율적이라고 생각해서 배열을 지양하시는게 어떨까 하고 드린 의견이었구요!
이건 대답을 안주셨는데, 44번 라인 빈 배열만 만들고 속이 안채워져있는 것 같아요~ 지금 프로그램 실행하면 NPE가 나지 않나요 ?
https://edu.nextstep.camp/s/RFY359FE/ls/CAoxYedP
자동차 경주 게임 요구사항에 맞춰 구현했습니다!