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단계 - 자동차 경주 구현] 허브(방대의) 미션 제출합니다. #510

Merged
merged 18 commits into from
Feb 9, 2023

Conversation

greeng00se
Copy link
Member

안녕하세요!

우테코 백엔드 5기 허브입니다. 🌿
저의 PR에 대한 리뷰를 남겨주시는 거에 대해 미리 감사의 말씀을 드립니다!!! 😄

추가로 몇가지 궁금증, 고민을 같이 첨부하여 PR 보냅니다.

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

미션 내 0 ~ 9 사이의 값 무작위 생성에 대한 테스트를 진행해야 하는 경우가 있었습니다.
결과가 10가지가 있을 수 있다고 생각하고 안정감을 가지기 위하여 @RepeatableTest를 이용하여 10번 반복해서 테스트를 하였는데
이 값이 많아지면 많아 질 수록 이런 방식으로 테스트 하기에는 어려움이 있다고 생각됩니다.

고민 -> 무작위 값에 대한 테스트를 어떻게 하면 개선할 수 있을까요?

  1. 위임 메서드에 대한 테스트

모든 원시값과 문자열을 포장(wrap)한다. 라는 객체지향 생활체조 원칙에 따라 값 타입을 포장해보았습니다. (이름, 위치, 시도 횟수)
이를 적용함에 따라 위임하는 형태의 메서드가 자주 생성이 되었습니다.
실제로 적용하지는 않았지만 이부분에 대해서 Mockito를 사용해서 해당 메서드가 내부에서 실행되는 횟수에 대한 검증을 하는 방법이 있다는 것을 알았습니다.

고민 -> 단순 위임만 하는 경우 결과를 검증하는 것, 내부 메서드의 호출 횟수를 검증하는 것 중 어떤 방법을 사용해서 테스트 하는 것이 더 나은 방법일까요?

  1. 올바른 동작 확인(테스트)을 위한 getter

시도 횟수를 표현하는 Count 객체에서 테스트를 위해 getValue를 정의하였습니다.
카운트를 1에서 0으로 낮추는 메서드를 실행하고 Count.isPlayable로 테스트를 작성하다가
테스트가 명확하다고 느껴지지 않아 getter를 생성해서 테스트를 진행했습니다.

고민 -> 원시값을 포장한 클래스에서 테스트만을 위한 getter를 사용해도 될까요?

  1. 입력의 검증, 적절한 형태로 변환

올바른 입력을 받는 것도 InputView의 책임이라고 생각하여 InputValidator 클래스에서 검증, 또한 비즈니스 로직에서 사용하기 편하게 변환까지 진행을 했습니다.

고민 -> 검증을 위한 클래스에서 타입 변환의 책임까지 가지는 형태가 되어, 어떤 방법으로 개선할 수 있을까요?

위와 같이 다양한 고민을 하며 진행을 했습니다!
고민에 대한 리뷰어님의 개인적인 의견이 궁금합니다!
부족한 부분, 개선을 할 수 있는 부분에 대해 리뷰 남겨주시면 정말 감사하겠습니다.

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>
Copy link

@knae11 knae11 left a comment

Choose a reason for hiding this comment

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

안녕하세요~ 허브 반갑습니다 :)
일급컬렉션에 값객체, 다양한테스트까지 미션 잘 수행해주셨네요 👍 1단계 내용에서는 충분히 잘 해주신 것 같아 merge 하겠습니다!

고민에 대한 제 의견 조금 남겨볼게요! 제 의견일 뿐 정답은 아니라는거 아시죠~?!ㅎㅎㅎ

  1. 제어할 수 없는 부분은 테스트도 진행하지만, 제어할 수 없는 부분에 문제가 생겼을 때 예외처리하는 부분을 검증하는 테스트를 진행하면 마음이 놓일 것 같네요! 무작위 생성값이 범위를 넘어가는 것에 대해 무한히 테스트하는 건 큰 의미가 없다고 생각해요. 어떤 부분을 테스트하고 싶은지에 대한 집중해서 테스트를 작성하면 되지 않을까요?
  2. 위임을 했다는 것은 역할과 책임을 넘겨주겠다는 건데요. 내부에서 몇번이나 호출하고 어떻게 하는지 신경쓰기 보다는 결과를 검증하는게 좋다고 생각합니다!
  3. 필요하다면 getter를 사용할 수 있다고 생각해요. 테스트만을 위해서 코드가 생성되는건 좋지 않긴 하지만요. Count(1)에서 decrease를 했을 때, playable 하지 않게 되는것도 의미가 있다고 생각합니다.
  4. 입력에서 비지니스 로직에 사용하기 편하게 전달해주면 로직을 수행하기 좋은데요~! (코드에서 타입의 변환이 이루어지고 있진 않은 것으로 보이는데요) 검증하기 위해 필요한 부분이라면 문자열 조작이 이루어질 수있다고 생각합니다. 검증단에 해당 로직을 넣고 싶지 않다면 문자열을 정리하는 역할의 클래스를 만들 수도 있지만요. 레이싱카 크기의 어플리케이션에서는 이런 부분까지 나눈다면 클래스가 지나치게 많다는 생각입니다.

RacingCarController --> OutputView
```

## 요구사항
Copy link

Choose a reason for hiding this comment

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

요구사항에 대한 todo list 👍 👍

findWinners(racingGame);
}

private RacingGame gameInitialize() {
Copy link

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.

리뷰해주신대로 동작을 하는 것이기 때문에 initialize로 하는것이 더 좋아보이네요!!
너무 감사합니다 😄

Comment on lines +31 to +33
RacingGame racingGame = gameInitialize();
play(racingGame);
findWinners(racingGame);
Copy link

Choose a reason for hiding this comment

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

간략하게 게임이 어떻게 흘러가는지 보여서 좋네요! 👍

import java.util.Comparator;
import java.util.List;

public class Cars {
Copy link

Choose a reason for hiding this comment

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

일급 컬렉션까지 👍

@knae11 knae11 merged commit 788a887 into woowacourse:greeng00se Feb 9, 2023
@greeng00se
Copy link
Member Author

안녕하세요 배럴!

평소에 현직자의 의견을 들을 기회가 많이 없었는데, 의견을 꼼꼼하게 남겨주셔서 정말 감사합니다.
하나씩 꼼꼼히 읽었는데, 머리속에 있던 안개가 걷히는 느낌이 들었습니다.
너무 좋은 의견을 받았기 때문에 페어와 더 좋은 이야기를 나눌 수 있을 것 같습니다!

우아한 테크코스를 경험하면서 이렇게 리뷰 받는 것이 정말 값진 경험인 것 같습니다!
다시 한 번 더 감사드리고, 2단계때 더 개선된 모습으로 찾아뵙도록 하겠습니다 😄

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.

3 participants