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단계 - 자동차 경주 구현] 홍실(홍혁준) 미션 제출합니다. #459

Merged
merged 35 commits into from Feb 11, 2023

Conversation

hong-sile
Copy link

No description provided.

kyY00n and others added 23 commits February 7, 2023 16:09
* 페어프로그래밍 연습
자동차의 이름의 유효성 검사 기능 포함
* 프로그램 메인 메서드에서 컨트롤러를 실행.
Copy link

@ohjoohyung ohjoohyung 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,37 @@
# 기능 요구 사항

Choose a reason for hiding this comment

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

요구사항 👍
예외 사항까지 꼼꼼하게 작성해주셨네요!
다만, 프로그램 흐름과 같은 항목은 규모가 커질수록 상대적으로 변경되기 쉬운 내용이라 이후 미션은 요구사항만 작성해도 괜찮지 않나 싶어요!

Copy link
Author

Choose a reason for hiding this comment

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

큰 규모에서는 확실히 변경이 어렵겠네요. 감사합니다

Comment on lines 7 to 18
public static final int LEAST_CONDITION = 4;
public static final int MAX_NAME_LENGTH = 5;
public static final int START_POSITION = 0;
public static final int ONE_STEP = 1;
private final String name;
private int position;
public static final Comparator<Car> positionComparator = new Comparator<Car>() {
@Override
public int compare(Car o1, Car o2) {
return o1.position - o2.position;
}
};

Choose a reason for hiding this comment

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

자바 코드 컨벤션을 지키면서 프로그래밍한다.

라는 요구사항이 있었죠? 아래 글을 통해서 상수, 변수 순서나 네이밍 규칙을 참고해주세요
그리고 왜 컨벤션을 지키는게 좋을지에 대해서도 생각해보셔도 좋습니다.
https://myeonguni.tistory.com/1596

Copy link
Author

Choose a reason for hiding this comment

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

Comprator positionComrpator를 리팩토링 할 때 넣어서 컨벤션을 생각하지 못했네요.
유의하면서 작성하도록 하겠습니다!

Comment on lines +27 to +32
if (name.isEmpty()) {
throw new IllegalArgumentException("자동차 이름은 공백일 수 없습니다.");
}
if (name.length() > MAX_NAME_LENGTH) {
throw new IllegalArgumentException("자동차의 이름은 다섯 글자 이하여야합니다.");
}

Choose a reason for hiding this comment

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

validation 👍
다만 ' ' 과 같은 공백일 때는 그대로 실행되던데 의도하신걸까요?
isBlank() vs isEmpty()에 대해서 찾아보셔도 좋습니다

Copy link
Author

Choose a reason for hiding this comment

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

해당라인에서 의도했던 것은 "hong,,rosie"같은 입력이 컴마(,)를 기준으로 split되어, ["hong", "", "rosie"]와 같은 경우에 대하여 유효성 검사를 하려고 했습니다.
오즈님이 리뷰해주신 것처럼 isBlanck로 바꾸면 공백으로 입력되는 경우도 포함하여 유효성을 검증할 수 있겠네요!!

Comment on lines 41 to 43
public CarDTO toDTO() {
return new CarDTO(name, position);
}

Choose a reason for hiding this comment

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

DTO를 만드셨군요!
View 영역에 제공될 객체인거 같은데 Domain 영역이 이걸 알고 있으면 어떤 상황이 생길 수 있을까요?
고민해보셔도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

getter를 쓰기 싫어서 dto를 Car 내부에서 생성했는데, dto를 도메인이 알고 있을 때 생기는 문제는 생각을 못했네요.

domain 레이어에 dto를 생성하여 반환할 경우, view에 보여주고 싶은 데이터를 변경할 때 domain을 변경하게 되는 상황이 생길 것 같아요. view가 domain에 의존적이라는 점에서 계층 간의 분리가 잘 안되어 있는 상태인 것 같네요.

이 리뷰 덕에 이전에 몰랐던 문제점을 알고 고민하는 과정에서 많이 배울 수 있었습니다. 감사합니다

Comment on lines 4 to 9
public final String name;
public final int position;
public CarDTO(String name, int position) {
this.name = name;
this.position = position;
}

Choose a reason for hiding this comment

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

Suggested change
public final String name;
public final int position;
public CarDTO(String name, int position) {
this.name = name;
this.position = position;
}
public final String name;
public final int position;
public CarDTO(String name, int position) {

공백이 한칸 더 들어갔네요ㅎㅎ
가독성을 위해 인스턴스 변수와 생성자 사이에 라인을 띄워주는 습관을 들여놓는 것도 좋습니다

Copy link
Author

Choose a reason for hiding this comment

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

이번 미션에서 컨벤션을 지키지 못한 것들이 좀 있네요.😥
잘 작성하도록 하겠습니다!

}

public List<CarDTO> judgeWinners() {
Car c = Collections.max(cars, Car.positionComparator);

Choose a reason for hiding this comment

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

변수명을 조금 더 고민해보시면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

우승자를 결정하는 로직을 계속 고민하면서 바꾸다 보니 대충 작성한 게 바로 들어갔네요.
더 더 신경쓰도록 하겠습니다.


@ParameterizedTest
@ValueSource(strings = {",,", ","})
@DisplayName("이름을 입력하지 않으면")

Choose a reason for hiding this comment

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

DisplayName을 조금 더 고민해서 작성해보는건 어떨까요??

Comment on lines 54 to 62
void throwIllegalArgumentExceptionWhenInputIsNotNumber(String input) {
//given
InputStream in = new ByteArrayInputStream(input.getBytes());
System.setIn(in);
inputView = new InputView();
//when,then
assertThatThrownBy(() -> inputView.readTryTime())
.isInstanceOf(InputMismatchException.class);
}

Choose a reason for hiding this comment

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

Input, Output에 대한 테스트까지..!!
다만, 실행해보니 테스트가 깨지네요! 리뷰 요청 전에 꼭 확인 부탁드릴게요 🙏

모든 로직에 단위 테스트를 구현한다. 단, UI(System.out, System.in) 로직은 제외

그리고 요구사항에는 InputView, OutputView에 대한 테스트는 제외라고 적혀있더라구요.
요구사항을 지키는 것도 중요하다는 것을 명심해주세요!

Copy link
Author

Choose a reason for hiding this comment

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

요구사항 미숙지에, 테스트 에러까지 수정해야 할 점이 많군요. 다시 리뷰 요청 보낼 때 유의하겠습니다.

return numbers.poll();
}
}
}

Choose a reason for hiding this comment

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

No newline at end of file 경고문은 어떤 걸까요? 한번 찾아보시고 해결해보면 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

ide에서만 코드를 봐서 이전에 해당 에러가 존재하는지도 모르고 있었습니다.
이번에 오즈님 덕분에 알게 되었네요. 감사합니다!!

Comment on lines 20 to 39
@BeforeEach
void setup() {
Race race = new Race(List.of("rosie", "hong"), new TestNumberPicker(4, 1));
race.tryMoveOneTime();
winners = race.judgeWinners().stream().map(car -> car.name).collect(Collectors.toList());
}

@Test
@DisplayName("반환값에 우승자가 포함되어 있는지 테스트")
void shouldContainsWinners() {
Assertions.assertThat(winners)
.containsExactly("rosie");
}

@Test
@DisplayName("반환값에 우승자가 아닌 사람이 포함되어 있지 않은지 테스트")
void shouldNotContainNonWinners() {
Assertions.assertThat(winners)
.doesNotContain("hong");
}

Choose a reason for hiding this comment

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

judgeWinners 메서드 테스트에 tryMoveOneTime 메서드도 같이 사용되어 있는데요.
각각의 메서드에 집중한 단위 테스트는 어떻게 작성할 수 있을까요? 또한 단위 테스트의 범위에 대해 고민을 해보셔도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

기존 코드는 judgeWinners 메서드 테스트는 judgeWinners메서드 뿐 아니라 tryMoveOneTime메서드에도 의존적이네요.
둘을 분리하여 테스트 코드를 작성하는 방식으로 변경하였습니다.

여기서 tryMoveOneTime 메서드를 테스트할 때, testNumberPicker를 사용하여, 테스트 때 tryMoveOneTime에 대한 멱등성을 보장했습니다.
하지만 메서드 실행 후 해당 메서드가 예상한대로 동작을 했는지 확인하기 위해 List->Car->position을 확인해야 하는데, 이를 어떻게 확인 할 수 있을까요?

제가 고민한 방법은 크게 두 가지가 있습니다.

  1. Race에서 List를 반환하는 get메서드를 정의하고, assertj의 extracting을 이용하여, tryMoveOneTime 메서드 실행 이후, position이 예측한대로 변화했는지 확인한다.
    -> 테스트에서만 사용하는 getCars메서드를 정의하는 것이 옳은 것인지 의문이 듭니다.

  2. mockito를 이용하여 car.moveDependingOn의 호출횟수를 확인한다.
    -> 현 미션에서는 mockito 라이브러리가 없으므로 불가능합니다.

etc. tryMoveOneTime은 이미 검증이 된 Car의 moveDependingOn을 호출하는 단순한 로직이기에 구현하지 않아도 된다.
-> 이런 방식으로 생각한다면, 어디까지가 테스트코드 작성범위인지 구분을 못할 것 같습니다.

혹시 위 두 방법 외에 어떤 방법이 있을까요? 힌트만 주신다면 혼자 더 고민해보겠습니다.

Copy link

Choose a reason for hiding this comment

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

벌써부터 이런 생각을 하시다니.. 👍
우선 단위 테스트(뿐만 아니라 모든 테스트)를 작성할 때 이 테스트는 무엇을, 어떤 관점으로 테스트 할지에 대해 생각을 해봐야 할 것 같아요.

Race의 tryMoveOneTime 메서드는 어떤 비즈니스 로직을 수행하나요? 반복문을 돌면서 저장된 car 객체에게 move 메시지를 보내는 행위를 한번 수행하는거죠? 그렇다면 이 메서드의 테스트를 통해 홍실은 어떤 것을 검증하고 싶은 걸까요?
단순 car가 한번씩 move를 수행했는지? 아니면 car들이 정말로 조건에 따라 한번씩 move를 수행했는지?
만약 한번씩 수행했는지를 검증하고 싶다 (행위 검증) 면 car를 mock 객체로 만들어서 말씀해주셨던 mockito 라이브러리를 사용해 호출횟수를 확인할 겁니다. 하지만 그게 아니라 car가 정말로 조건일 때 move를 수행했는지를 테스트해주려면 car 리스트를 가져와 각각의 position을 검증 (상태 검증) 했을 거예요.
저는 개인적으로 레벨1 미션 동안은 mock 객체, 행위 검증 등을 생각하지 않으셨으면 해요. (사실 그렇게 해야겠다고 생각이 들 미션도 없을 거예요)
도메인 객체들이 협력을 통해 비즈니스 로직을 정상적으로 수행했는지, 실패했는지 이런 것을 집중해서 테스트를 작성해주시면 좋을 것 같습니다 😄

그리고 1번 같은 경우 홍실이 이미 getStatuses 라는 메서드를 만드시고 프로덕션 코드에서 사용하고 있어요! 따라서 테스트에서만 사용하진 않다고 봅니다.
etc에서 언급해주신 부분의 경우, 이미 검증이 되었다고 해도 둘 다 테스트를 작성해주어야 한다고 생각합니다. 각각의 테스트가 하는 역할이 다르니까요ㅎㅎ 요구사항이 변경되어 tryMoveOneTime에서 단순히 car의 move를 호출하는게 아니게 된다면..?
이런 부분도 생각해 볼 수 있겠죠?

Copy link
Author

@hong-sile hong-sile Feb 13, 2023

Choose a reason for hiding this comment

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

와우 헷갈려하던 거에 대한 해답이 되었네요. 오즈 덕분에 어느정도 기준을 잡을 수 있을 것 같아요 너무 감사합니다.

@hong-sile
Copy link
Author

늦은 시간임에도 리뷰를 작성해주셔서 너무 감사합니다.
제가 이번에 코드를 작성하면서, 계속 고민했던 것이 있는데 해당 내용에 대해서 오즈의 의견을 말해주면 감사하겠습니다.

저는 이번 테스트 코드를 위하여, position을 parameter로 받는 Car의 생성자, NumberPicker 인터페이스와 구현체들 등, 프로덕션 코드에서 실 기능과 무관한 코드를 몇몇 만들었습니다.
저는 테스트 코드를 위해 프로덕션 코드의 구현 또는 설계가 변경될 수 있다고 생각합니다. 하지만 어디까지 변경해도 되는지에 대한 기준을 세우지 못하겠습니다.

"테스트 코드를 위해 프로덕션 코드를 어디까지 수정해도 되는지?"에 대한 의견을 한번 말해주시면 감사하겠습니다.

Copy link

@ohjoohyung ohjoohyung left a comment

Choose a reason for hiding this comment

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

안녕하세요 홍실!
피드백 잘 반영해주셨어요. 👍
질문에 대한 답변 및 몇가지 코멘트 추가로 남겼습니다.
1단계는 여기서 끝내고 2단계에서 반영해주시면 좋을 것 같아요.
수고하셨습니다!

outputView.printWinners(toListCarDto(race.getWinners()));
}

private List<CarDto> toListCarDto(List<Car> cars) {

Choose a reason for hiding this comment

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

toListCarDto의 경우 만약 list에서 다른 collection을 반환해주도록 변경되면 똑같이 이름을 변경해줘야겠죠?
복수형과 같은 표현은 어떨까요?

Copy link
Author

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 +24
public Car(String name) {
validateName(name);
this.name = name;
this.position = START_POSITION;
}

public Car(String name, int position) {
validateName(name);
this.name = name;
this.position = position;
}

Choose a reason for hiding this comment

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

동일한 생성자 흐름인데 중복을 줄일 수 있지 않을까요?
this()에 대해 찾아보시는 것도 좋을 것 같아요

Comment on lines +32 to +36
public void tryMoveOneTime(NumberPicker numberPicker) {
for (Car car : cars) {
car.moveDependingOn(numberPicker.pickNumber());
}
}

Choose a reason for hiding this comment

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

이전에 언급드리진 않았지만 전략 패턴을 잘 사용해주셨어요 👍

src/main/java/domain/RandomNumberPicker.java Show resolved Hide resolved
Comment on lines 49 to 54
//given
Race race = new Race(List.of("rosie", "hong"));
//when
race.tryMoveOneTime(new TestNumberPicker(4, 1));
//then
//Mock 라이브러리를 이용하여 검증

Choose a reason for hiding this comment

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

오오.. Mock 라이브러리에 대해서 생각하시다니 👍
다만 지금은 꼭 이러한 라이브러리를 이용해 검증할 필요가 있을까요?
테스트의 관점에 따라 다를 수도 있겠지만 자동차가 움직였는지 아닌지는 position을 통해 검증할 수 있지 않을까요?

@Nested
class ReadTryTimeTest {
@DisplayName("integer 범위의 수가 아닌 문자열")
@ParameterizedTest(name = "\"{0}\" 인 케이스 일때 InputMismatchException 발생")

Choose a reason for hiding this comment

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

테스트가 변경되었으니 name도 수정해주세요!

@ohjoohyung
Copy link

늦은 시간임에도 리뷰를 작성해주셔서 너무 감사합니다. 제가 이번에 코드를 작성하면서, 계속 고민했던 것이 있는데 해당 내용에 대해서 오즈의 의견을 말해주면 감사하겠습니다.

저는 이번 테스트 코드를 위하여, position을 parameter로 받는 Car의 생성자, NumberPicker 인터페이스와 구현체들 등, 프로덕션 코드에서 실 기능과 무관한 코드를 몇몇 만들었습니다. 저는 테스트 코드를 위해 프로덕션 코드의 구현 또는 설계가 변경될 수 있다고 생각합니다. 하지만 어디까지 변경해도 되는지에 대한 기준을 세우지 못하겠습니다.

"테스트 코드를 위해 프로덕션 코드를 어디까지 수정해도 되는지?"에 대한 의견을 한번 말해주시면 감사하겠습니다.

정말 좋은 고민입니다 👍
우선 홍실이 작성을 해주신 코드를 기준으로 먼저 말씀드려볼게요.

  • postion을 parameter로 받는 Car의 생성자
    • 개인적인 생각으로 상황에 따라 다르겠지만 테스트를 위해 생성자를 추가하는 건 괜찮다고 봐요. 테스트 코드를 작성하는 것은 매우 중요하고 테스트 코드 또한 프로덕션 코드만큼 높은 품질을 유지하는게 좋으니까요. 예를 들어 이번에 postion을 parameter로 받는 Car 생성자를 추가함에 따라 Car 객체의 position을 원하는 값으로 초기화할 수 있어 테스트에서 매번 move를 통해 세팅해줄 필요가 없어지겠죠? 따라서 테스트를 작성하기 수월해지고 유지보수 하기도 편해질 것으로 생각돼요.
      다만, 추가된 생성자로 인해 기존 정책에서 벗어난 객체를 만들면 안된다고 생각합니다. 홍실이 이번에 추가해준 Race 객체의 경우 Cars 배열을 받고 validation이 없는 생성자를 추가해주셨는데 이렇게 되면 무슨 일이 발생할 수 있을까요? CarName 리스트를 받는 생성자와 validation이 달라 처음 보는 사람은 무엇이 정책인지 헷갈릴 수 있지 않을까요? 이런 점들을 한번 생각해주시면 좋을 것 같아요. (라고 작성했는데 코드가 수정되었네요...ㅎㅎ;; 그래도 그대로 둘테니 참고만 해주세요)
  • NumberPicker 인터페이스와 구현체
    • NumberPicker 인터페이스를 만든건 좋은 방법이라고 생각합니다. 요구사항에 대한 확장성을 고려할 수 있고 또한 이를 통해 테스트에서 원하는 값을 만들어 랜덤 값을 제어할 수 있게 되었으니까요. 덕분에 모든 비즈니스 로직에 대한 단위 테스트를 작성할 수 있게 되었죠?

제 생각이고 '어디까지'에 대한 정답은 없습니다만, 테스트를 위해 기존 요구사항, 비즈니스 로직에 벗어나지 않고 어느정도 용인이 가능한 (ex 생성자) 코드를 추가하는 것은 괜찮다고 생각해요. 다만 만약에 테스트 코드를 작성할 때 어려움을 느껴 정말 필요없는 부분을 추가, 수정 해야하는 코드가 생긴다면 혹시나 설계가 잘못되진 않았을까와 같은 접근이 필요하지 않을까 싶어요. 좋은 테스트 코드, 프로덕션 코드를 고민하여 설계하고 작성하기 위해 TDD라는 방법론까지 존재하니까요 😄

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.

None yet

3 participants