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단계 - 자동차 경주 구현] 다즐(최우창) 미션 제출합니다. #504

Merged
merged 18 commits into from
Feb 9, 2023

Conversation

woo-chang
Copy link
Member

@woo-chang woo-chang commented Feb 9, 2023

안녕하세요 😄 다즐입니다!

웹 백엔드 레벨 1단계 미션인 자동차 경주에 대해 구현해봤습니다. 진행 도중 몇 가지 궁금한 점이 존재하여 질문 남깁니다.

제어할 수 없는 부분에 대한 테스트

미션에서는 랜덤으로 값을 생성하는 기능이 필요하여 RandomNumberGenerator라는 도메인을 구현해보았습니다.

해당 도메인의 메서드인 generate는 제어할 수 없는 Random에 의존하고 있기에 테스트를 검증하기 어려웠고, 0~9 사이의 값을 가지는 만큼 10번의 테스트를 통해 생성된 값이 범위안에 속하는지 확인하여 적절한 기대값을 가지는 테스트 케이스를 작성해 보았습니다.

이런 경우 어떻게 테스트를 진행해야 하는지 궁금합니다!

위임 메서드에 대한 테스트

RacingGame 도메인에 게임이 진행할 수 있는 상태인지 확인하는 isPlayable 메서드를 정의하였습니다.

게임이 진행할 수 있는 상태를 확인하기 위해서는 필드인 Count에게 역할을 위임해야 했고, RacingGame의 isPlayable 메서드는 단순히 역할을 위임하는 위임 메서드가 되었습니다.

이런 경우 Count, RacingGame 두 메서드 모두 테스트를 작성해야 하는지 궁금합니다!

테스트를 위한 Getter

Count 도메인은 시도 횟수를 줄이기 위한 decrease 메서드를 가지고 있습니다.

해당 메서드의 결과를 확인하기 위해서는 Count의 Getter를 열어두어야 하는데, 테스트 검증을 위해 Getter를 여는 것이 맞는지 궁금합니다!

입력에 대한 검증

입력에 대한 검증을 위해 InputValidation을 구현하였습니다. 해당 객체에는 검증에 대한 역할만 부여하려고 하였으나, 검증 과정에서 파싱이 발생하고 검증이 끝난 후에도 파싱이 필요해져 중복되는 코드로 인해 검증 후 올바른 결과까지 반환하도록 메서드를 작성하였습니다.

이 부분은 많은 고민을 하였으나 중복되지도 않고 역할도 분리할 수 있는 방법이 떠오르지 않아 질문으로 남기게 되었습니다. 어떻게 생각하시는지 궁금합니다!

리뷰 부탁드리겠습니다. 감사합니다.⛄️

woo-chang and others added 18 commits February 8, 2023 13:34
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
Co-authored-by: greeng00se <thegreengoose7@gmail.com>
@sihyung92
Copy link

sihyung92 commented Feb 9, 2023

3번 질문이었던

위임 메서드에 대한 테스트

에 대한 답변입니다.

네. 필요합니다. 왜냐하면 RacingGame이 내부적으로 Count에게 위임하고 있다는 것은 캡슐화 되어있기 때문입니다. (RacingGame을 사용하는 사람은 그 사실을 모릅니다) 테스트는 해당 클래스가 표현하는 행위를 검증해야 하지, 내부 구현을 검증하려 하면 구현이 조금만 바뀌어도 테스트를 수정해야 하는 코드가 됩니다.

Copy link

@sihyung92 sihyung92 left a comment

Choose a reason for hiding this comment

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

안녕하세요 다즐!
커밋 내역을 상세히 분리해주셔서 리뷰가 수월했습니다. 감사해요 🙇🏻 자바 사용과 클래스 분리, 그리고 테스트가 능숙하신 걸 느낄 수 있었어요. 2단계 리뷰 요청이었다고 생각해도 믿었겠네요.
워낙 잘 해주셨기 때문에 바로 머지하겠습니다! 다음에 만나요~~ 💪


## 그래프

```mermaid

Choose a reason for hiding this comment

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

mermaid를 통해 객체 그래프를 그려주셔서 전체적인 구조를 파악하기 용이했습니다. 감사합니다 🙏


## 요구사항

Car

Choose a reason for hiding this comment

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

각 객체별로 수행하고 있는 역할을 잘 작성해주셨습니다.

다만 구현하신 모든 객체의 역할을 명시하는 방식이 이어질 미션들에서도 지속가능한 것인가? 고민해보셨으면 좋겠어요. ☺️ 조금 더 기능이 많아지고 객체가 많아졌을 때, 어떤 방식으로 요구사항 명세서를 작성하시면 좋을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

객체별로 기능을 작성하면서 어느 순간 객체에 의존적인 기능들만 작성하는 느낌이 들긴 하였습니다. 객체에 초점을 맞추기보다는 메세지에 조금 더 집중하여 기능 중점적인 요구사항을 작성해보는 것도 좋을 것 같아요 😄

기능 중심 요구사항을 작성하기 위해서는 객체 이름을 명시하지 않아야 할 것 같은데, 한편으로는 명세서를 봤을 때 어떤 객체가 어떤 동작을 하는지 의아해할 수도 있을 것 같아요. 웨지는 어떤 방식으로 요구사항 명세서를 작성하시나요?

Choose a reason for hiding this comment

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

아 요구사항 명세서 보고 느낀게 있다고 말씀드렸는데 이미 이틀전에 제가 말씀드렸군요 ㅋㅋ 2일전의 저와 현재의 저가 느낀 점이 다르지 않아서 신기하네요 ^^; 건망증도 신기하고요

요구사항 명세서를 왜 쓸까요?
글을 쓸 때는 주제와 목적, 화자를 첫 번째로 고려해야 합니다. 요구사항 명세서도 글이니 크게 다르지 않겠죠.
주제는 "요구사항 명세"로 확정되어 있으니 목적과 화자를 고려해봅시다.
(물론 "미션 요구사항에 맞추기 위해서"가 1번 목적이시겠지만, 이러면 리뷰어 입맛에 맞추는 뻘글이 되므로 좀 더 의미부여를 해봅시다 ㅋㅋ)

제가 이 미션을 하는 입장이었다면 미션의 README를 작성할 때 "미래의 내가 이 글을 다시 읽고 시스템을 파악할 수 있는가?" 를 목적으로 삼겠습니다. 이 스레드에서 보셨다시피 저는 기억력이 이틀을 못 가기 때문에 저는 기억을 깡그리 잊은 상태로 이 글을 다시 보게 될 것이고요, 자연스럽게 화자는 {기억을 잃은 나 == 맥락을 모르는 개발자} 가 되겠죠.

제가 '맥락을 모르는 개발자가' '시스템을 파악해야 할 때' 안내 글을 쓰는 스타일은

  1. 맥락을 먼저 적고, "이 게임은 레이싱 게임입니다. n명의 유저가..."
  2. 아래에 세부사항을 적습니다. "자동차는 이름과 위치를 가질 수 있습니다... "
  3. 세부사항을 적을 땐 동작해야 하는 기능 외에도 예외사항들을 최대한 모두 적습니다. "이름은 n자 이하여야 합니다.."

위와 같습니다.

이런 제 입장에서 볼 때 아래의 Name과 Position에 대한 설명은 다소 아쉽습니다. 무슨 이름인지 맥락이 없어서 처음 보는 사람이면 결국 코드로 들어가서 찾아봐야 되거든요. (Name이 어디어디 들어가나?)
저라면 Car에 하위 뎁스에 집어넣어서, 이게 차의 이름이라는 걸 명확히 할 거 같습니다.

말이 워낙 길었는데요 ^^; 이건 정답은 없는 문제입니다. 다즐도 이 글을 어떤 목적으로 작성하는지 생각해보시고, 그에 맞게 작성해주시면 됩니다. 다만 제 입장에선 모든 클래스의 설명이 명시된 요구사항 명세서는 쫌 과하다는 생각이 들 것 같습니다 ㅎㅎ

Copy link

@sihyung92 sihyung92 Feb 11, 2023

Choose a reason for hiding this comment

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

다시 읽어보니 리뷰가 개념 2개를 섞었네요 ㅋㅋ 정리하자면

  1. 모든 클래스가 하는 기능을 일일이 명시한 요구사항 명세서는 읽는 사람 입장에서 불편할 수 도 있습니다.
  2. 연관이 있는 개념끼리는 묶어서 정리해주시면 이해가 수월합니다.

이렇게 될 거 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

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

명세서는 누구나 볼 수 있는 글인데 작성한 사람만 이해할 수 있는 글, 더 확장하면 작성한 사람마저 이해할 수 없는 글을 작성해버렸네요 .. 반성하게 됩니다 😂

프리코스 과정에서도 명세서를 작성하며 개발했지만, 왜 작성하는지에 대한 고민은 깊게 하지 못했던 것 같아요. 늘 가던 길로만 가다가 목적지에 효율적으로 갈 수 있는 새로운 길을 찾은 느낌이 들어요 .. !

직접 경험하신 내용을 전달해주셔서 너무 감사합니다. 정말 많이 배울 수 있엇습니다 👍👍

this.outputView = outputView;
}

public <T> T retry(final Supplier<T> supplier) {

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 +8
private Name name;
private Position position;

Choose a reason for hiding this comment

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

final을 통해 객체 재할당을 막아주면 어떨까요? ☺️

Copy link
Member Author

Choose a reason for hiding this comment

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

꼼꼼히 확인한다고 했으나 놓친 부분이 존재하네요 .. 정말 좋은 것 같습니다 👍

this.value = name;
}

private void validate(String name) {

Choose a reason for hiding this comment

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

인자에 final을 붙일 때도, 안 붙일 때도 있는데 특별한 의도가 있으실까요? 🙏

Copy link
Member Author

@woo-chang woo-chang Feb 10, 2023

Choose a reason for hiding this comment

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

모든 인자에 final을 붙이는 것으로 페어와 얘기하고 진행했으나, 리팩토링을 진행하며 놓친 부분에 해당합니다 .. 😂

import java.util.List;
import racingcar.domain.NumberGenerator;

public class TestNumberGenerator implements NumberGenerator {

Choose a reason for hiding this comment

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

테스트용 구현체 👍


@RepeatedTest(10)
@DisplayName("generate 메서드는 0 부터 9 사이의 숫자를 무작위로 생성한다.")
void should_returnNumber_betweenZeroToNine_when_generate() {

Choose a reason for hiding this comment

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

1번 질문에 대한 답변입니다.
값이 Random하게 흔들리거나 Time 의존적인 로직들은 상태가 매번 변하기 때문에 테스트하기 어렵습니다. 그런 로직들을 테스트 용이하게 만들려면 구현해주신 것처럼 값을 주입(DI)해줌으로써 해소할 수 있습니다.

구현을 분리하신 결과, RandomGenerator는 내부에 Random을 이용해 값을 생성하는 로직만을 가진 객체가 되었습니다. 이 객체에 대한 테스트는 Random, Java SDK에 대한 테스트가 될 수 있는데요.
단위테스트가 잘 구현되면 좋지만 Java SDK 기능까지 테스트의 영역에 포함되지는 않습니다. 해당 기능들은 역사가 보증하는 기능들이기 때문입니다. 그러므로 1번 질문에 대한 제 의견은 "여기까지 테스트 할 필요는 없다" 입니다.

만약 특정한 API를 사용해보지 않아 학습 목적으로 테스트를 작성하셨다면(학습 테스트), 본인이 API의 사용을 익히고 동료 개발자들에게 동작을 확인시켜주는 가치가 있습니다.


@Test
@DisplayName("decrease 메서드는 횟수를 1 감소시킨다.")
void should_minusOne_when_decrease() {

Choose a reason for hiding this comment

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

3번 질문에 대한 답변입니다. 저는 구현해주신 부분에서 decrease의 값을 검증하기 위해 getValue()가 필요하다고 생각하지 않습니다. 바로 밑 should_returnState_when_callIsPlayable() 테스트에서 csv Source 케이스만 몇 개 더 추가해주셔도, "입력된 value가 decrease할 때마다 1씩 소모되어 플레이가 불가능한 상태(0)에 이른다" 라는 Count의 기능을 충분히 테스트 가능하다고 생각합니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

좋은 의견 감사합니다 :) 처음에 말씀해주신 방법으로 진행하려고 했으나, 테스트를 위해 다른 메서드를 사용하게 되어 사용된 메서드에 의존성이 생기는 문제가 발생한다고 페어와 얘기하고 수정하였습니다.

테스트를 위해 다른 메서드를 사용하는 것이 맞는지 웨지의 의견이 궁금합니다 🤔

Choose a reason for hiding this comment

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

네 getter 또한 메서드 아닌가요? ㅎㅎ

아마 질문하신 의도는 두 메서드가 서로에게 의존적인데 이 경우 어떻게 테스트해야할지 여쭤보신거 같습니다. 대부분의 경우 어느정도 감안하고 테스트를 짭니다. 테스트를 위한 public 테스트를 여는것보단 낫다고 생각해용.

테스트를 위한 public 코드의 문제점은 좋은 코드, 나쁜 코드 라는 책 p355를 인용할게요.

  1. 실제로 우리가 신경쓰는 행동을 테스트하는 것이 아님
  2. 테스트가 세부 구현에 독립적이지 못 하게 됨
  3. 퍼블릭 API를 하나 추가한 꼴이므로, 다른 개발자가 해당 메서드에 의존하게 되어 수정과 리팩토링이 어려워짐

하지만 이번 미션에선 두 메서드가 항상 함께 호출된다면, 사실 한가지 역할 아닌가? 고민해보셔도 좋을거같아요. ☺️

private static final String INVALID_COUNT_MESSAGE = "횟수는 {0}이상의 정수만 가능합니다.";
private static final int COUNT_LOWER_BOUND = 1;

public List<String> validateNames(final String input) {

Choose a reason for hiding this comment

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

4번 질문이었던 InputValidation에 관한 역할분리에 대한 답변입니다.
질문주신 것처럼 InputValidator는 validate를 하면서 parse도 하고 있습니다. 그래서 해당 시그니처는 다소 어색하게 보입니다. (검증하는데 문자 컬렉션이 나오네?)

메서드는 하나의 역할을 하는 것이 바람직하나, 두 가지 역할을 하는 것 보다 더 나쁜 것은 두 가지 역할을 하면서 한 가지를 감추는 것입니다. 그러므로 저는 validateAndParseName같은 식으로 메서드명을 풍부하게 (차악을 선택) 만들거나,
오늘따라 역할 분리에 심취하고 싶은 날이라면 Parser와 Validator를 분리해 Parser가 Validator를 의존하는 방향으로 구현할 것 같습니다.


private Car findWinner() {
return cars.stream()
.max(Comparator.comparingInt(Car::getPosition))

Choose a reason for hiding this comment

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

getPosition()이 꼭 필요했나? 생각해보게 되네요.
해당 로직을 보며 compareTo()를 구현해 두었다면 Comparator의 도움 없이 정렬이 가능했을거란 생각이 들었습니다.
마침 테코블에 비슷한 개념을 가지고 구현을 한 사례가 있으니 참고해보셔도 좋을 것 같아요.

Copy link
Member Author

@woo-chang woo-chang Feb 10, 2023

Choose a reason for hiding this comment

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

Comparator를 사용할 때 Comparable이 떠올랐어야 했는데 .. 꼼꼼하게 리뷰해주셔서 정말 많이 배워갑니다!

감사합니다 ⛄️

Choose a reason for hiding this comment

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

네에~~ public accessor API(= getter)를 열어놓은 순간부터 해당 필드에 대해 타 객체들과의 의존성이 생겼다고 봐야해용
이게 나쁜건 아니지만 isSamePosition() 같은 메시지를 구현하여 "메시지 중심의 객체"를 만들려고 했던 시도들이 살짝 퇴색된 느낌이 있네요 ^^;

@sihyung92 sihyung92 merged commit 2090717 into woowacourse:woo-chang 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