Skip to content
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

[1단계 - 자동차 경주 구현] 채채(신채원) 미션 제출합니다. #472

Merged
merged 36 commits into from
Feb 9, 2023

Conversation

chaewon121
Copy link

@chaewon121 chaewon121 commented Feb 9, 2023

채채와 말랑 미션 제출합니다.
잘부탁드립니다!

@chaewon121 chaewon121 changed the title [STEP1 - 자동차 경주 구현] 신채원 미션 제출합니다. [1단계 - 자동차 경주 구현] 채채(신채원) 미션 제출합니다. Feb 9, 2023
Copy link

@dks301 dks301 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 채채, 리뷰어 시카입니다. 😄

요구사항에 맞춰 잘 구현해주셨네요! 👍
몇 가지 코멘트 남겨드렸으니 확인해주시고 다음 미션에서 반영해주세요!
추가로 궁금한 점이 생기면 언제든지 DM 주세요 ㅎㅎ

@@ -0,0 +1,31 @@
## 📈 기능 목록
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제 구현해주신 많은 기능에 맞춰 TODO LIST도 유지보수해보면 어떨까요??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번에 그 점이 부족하다고 생각하여 아쉬워 하였습니다. 다음 미션에서 더 신경쓰겠습니다!


public class Cars {

private static final int FIRST_INDEX = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final int FIRST_INDEX = 0;
private static final int WINNER_INDEX = 0;

상수 처리👍
다만, 조금 더 의미있는 이름을 사용해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

더 의미가 생긴거같습니다!


public Cars(final List<String> cars) {
if (cars.size() < MIN_SIZE) {
throw new IllegalArgumentException();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의미있는 메세지를 추가해 보는건 어떨까요???

Comment on lines +5 to +10
private final int totalLap;
private int currentLap;

public Lap(final int totalLap) {
this.totalLap = totalLap;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int는 0으로 초기화 되지만 명시적으로 넣어주면 어떨까요??

Comment on lines +8 to +11
public static String inputCarNames() {
System.out.println("경주할 자동차 이름을 입력하세요(이름은 쉼표(,)를 기준 기준으로 구분).");
return sc.nextLine();
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이름 입력에도 검증을 넣어주면 어떨까요??

Comment on lines +14 to +17
System.out.println("시도할 횟수는 몇회인가요?");
String input = sc.nextLine();
validateNumber(input);
return Integer.parseInt(input);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외가 발생해도 입력을 다시 받을 수 있도록 할수는 없을까요??

Comment on lines +24 to +29
@Nested
@DisplayName("성공 테스트")
class SuccessTest {

@ParameterizedTest(name = "createCars() 시 자동차 이름들을 입력받아(ex: {arguments}) 자동차를 생성한다.")
@MethodSource("racingcar.controller.RacingCarControllerTest#createCarSuccessParam")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

계층 구조 테스트👍
Junit 활용이 능숙하네요 💯

Comment on lines +68 to +76
/**
* success_test_3_version_1 과
* (cars.moveCars()의 호출 여부만을 검증,
* cars.moveCars()는 Cars 테스트를 통해 정상동작함을 확인할 수 있기 때문에 이곳에서는 호출만을 검증한다.)
* <p>
* success_test_3_version_2(전체 동작이 잘 이루어지는지 검증)
* <p>
* 의 방법 중 어떤 것이 더 좋다고 생각하시나요?
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

벌써 이런 부분을 고민하고 계시는군요 👏👏

테스트의 목적에 대해서 조금 더 고민해보시면 좋을 것 같아요.
ControllerTest에서

  1. 컨트롤러의 기능만 테스트하기 위한것
  2. 전반적인 기능을 통합테스트 하기 위한것

둘 중 어떤 것이 더 좋다라는 것 보단 상황과 목적에 따라 장단점을 파악해 선택적으로 사용하면 될 것 같아요.
이 부분은 앞으로도 고민하고 상황에 따라 선택해야하는 부분이기때문에 이 글 참고 하시면 좋을 것 같습니다 ㅎㅎ

// given
Car winner1 = new Car("말랑");
Car winner2 = new Car("채채");
Car nonWinner1 = new Car("시카");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢


// when & then
Assertions.assertThatThrownBy(lap::next)
.isInstanceOf(IllegalStateException.class);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

명확한 예외 메세지가 추가되면, 테스트에서도 확인할 수 있으면 좋을 것 같아요!

@dks301 dks301 merged commit 483b509 into woowacourse:chaewon121 Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants