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단계 - 자동차 경주 구현] 후니(최재훈) 미션 제출합니다. #270

Merged
merged 38 commits into from
Feb 14, 2022

Conversation

jayjaehunchoi
Copy link

안녕하세요 ! 후니입니다.

처음으로 리뷰를 받게 됐네요!!! 시간내셔서 리뷰해주시는 만큼 저도 열심히 코드 작성해보겠습니다!!
코드를 작성하며 몇 가지 질문사항이 있어 추가적으로 적어 보겠습니다!

  1. MockRandomGenerator를 추상 클래스로 만들어서 상속받아 CarTest, CarsTest에서 사용했는데, 이런 방식으로 Mock을 사용하는 것이 괜찮을지? 더 좋은 방법이 있을까요?

  2. Mockito 라이브러리 도움 없이도 가짜 객체를 만들거나 임의의 결과를 낼 수 있는 방법이 있을까요?

  3. static 메서드가 메모리를 일반 메서드보다 과하게 사용할 수 있을 것 같은데 static을 최소화하는 것이 좋을까요?

  4. static 으로 Scanner를 생성하여 사용해보니 개별 테스트에선 괜찮았으나 전체 테스트를 돌릴 때 NoSuchElementException이 터지며 No line found라는 메시지가 발생했습니다.
    그래서 Console 객체를 만들어 Reflection API 로 접근하여 Scanner가 없거나 닫혔을 때 scanner를 생성하여 반환하는 클래스를 만들었는데, 이때 Reflection을 사용하는 것에 대해 어떤 생각을 갖고 계신지 궁금합니다.

감사합니다!!
후니 드림

Copy link

@pkch93 pkch93 left a comment

Choose a reason for hiding this comment

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

안녕하세요 후니~! 후니의 첫번째 리뷰를 맡게된 철시입니다 ㅎㅎ
여러가지 고민들을 하시면서 다양한 기술들을 활용하셨네요 :) 여러 예외상황에 대한 처리와 핵심 요구사항에 대한 꼼꼼한 테스트가 인상적이네요 !
몇가지 코멘트 남겼는데 한번 확인해보시고 궁금한점 있다면 코멘트나 DM 주시면 답변드릴께요!


질문에 대한 제 생각을 남겨볼께요 :)

  1. 테스트하기 어려운 요인을 가짜 객체 Mock로 만들어서 테스트하겠다는 생각 좋습니다! 💯
    MockRandomGenerator는 여러 테스트 클래스에서 활용되는 기능들을 재활용할 수 있도록 기능들을 정의한걸로 보여요. 맞을까요? 추상클래스에 재활용할 기능들을 정의하여 활용하는 부분은 좋다고 생각합니다 :)

  2. 객체지향을 잘 활용하면 Mock 라이브러리의 도움 없이도 가능해요. 가능하면 우테코 미션을 하시면서 테스트를 작성하실때 Mockito와 같은 라이브러리를 사용하지는 않았으면해요. Mockito는 Mock을 만들어 줄때 Reflection을 활용하는데요. 때문에 애플리케이션 구조가 잘 설계됐든 잘못 됐든 상관없이 가짜객체를 통해 테스트가 가능하게 만들어줘요. (다른 Mock 라이브러리를 사용해보지는 않았어서 확실하지는 않지만 비슷하지 않을까 싶네요.)

테스트는 구조에 대해서도 피드백을 준다고 생각하는데요. 예를들어 전체 테스트가 실패한다는 피드백을 후니에게 줬을꺼에요. 이를 보고 어떤 문제가 있는지, 애플리케이션의 구조를 개선해서 해결할 수 있는지도 한번 고민해보면 좋을거 같아요.

  1. 메모리 관점에서 static 메서드나 일반메서드는 JVM의 Method 영역에 존재하는데요. static을 일반 메서드로 변경하더라도 결국 그 정보들도 메모리에 저장될거기 때문에 큰 차이는 없을거 같아요.

JVM 메모리 구조에 대해서 시간되실때 한번 학습해보시면 좋을거 같아요 :)

Naver D2 JVM Internal: https://d2.naver.com/helloworld/1230

다만, static을 사용했을때 여러 단점들이 있는데요. 가장 큰 단점이 객체지향의 다양한 장점들을 활용할 수 없다는 없다는 부분일 거 같아요. 객체간의 협력을 통해 애플리케이션을 구성하는 객체지향에서 static을 객체를 의존받을 수 없고 추상화 등과 같은 기능을 활용할 수 없죠.

정적 메소드, 너 써도 될까? 를 참고해보시면 좋을거 같아요!

  1. 전체 테스트를 돌리다보니 다 돌아가기 전에 Scanner가 close 되어서 이후 테스트에 영향을 미친거군요.
    테스트는 각 테스트가 환경에 독립적으로 실행이 될 수 있어야 하는데요. 현재는 전체 테스트에서 테스트간 영향이 있는 상태라 수정이 필요한 상황이 아닐까 싶어요.

[Effective Unit Testing] Chap2. 좋은 테스트란? - 2.4 독립적인 테스트는 혼자서도 잘 실행된다.를 참고해주세요 :)

때문에 수정을 위해서 Scanner에 sourceClosed라는 값을 Reflection을 사용해서 가져오고 있네요. 이렇게 Reflection을 사용하게 되면 현재 애플리케이션의 구조에 문제가 있든 없든 상관없이 기능을 구현할 수 있도록 만들 수 있어요. 때문에 정말 Reflection 사용이 필요한지에 대한 고민이 먼저 선행되어야한다고 생각합니다.

원인과 해결 방법을 고민해보신거 같은데 Reflection을 사용하지 않고 구현해본다면 애플리케이션의 구조가 더 개선될거 같아요!

src/test/java/racingcar/utils/MockRandomGenerator.java Outdated Show resolved Hide resolved
src/main/java/racingcar/utils/Validator.java Outdated Show resolved Hide resolved
src/main/java/racingcar/domain/Cars.java Outdated Show resolved Hide resolved
src/main/java/racingcar/domain/Cars.java Outdated Show resolved Hide resolved
src/main/java/racingcar/utils/Console.java Outdated Show resolved Hide resolved
src/main/java/racingcar/utils/Validator.java Outdated Show resolved Hide resolved
@@ -0,0 +1,18 @@
package racingcar.utils;

public class StubNumberGenerator implements NumberGenerator{
Copy link
Author

Choose a reason for hiding this comment

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

1명이 우승하는 상황과 2명이 우승하는 상황 모두 테스트 하기위해 이렇게 클래스를 수정해봤는데 이런 방법은 어떤가요?

Copy link

@pkch93 pkch93 Feb 14, 2022

Choose a reason for hiding this comment

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

테스트용으로 StubNumberGenerator를 작성해주셨군요 👍
다만, 현재 StubNumberGenerator의 반환값이 sequence에 의존하고 있네요. 아마 Cars의 생성자 Cars(String[] values)는 항상 Car의 position을 0으로 만들어주고 moveCars를 통해서 position을 변경시키려고 하기 때문에 sequence로 해결을 시도하신걸로 보이네요.

sequence가 추가로 테스트가 필요하여 작성한다고 했을때 ex. 우승자가 없을때, 3명 중 2명 우승 등 테스트 작성을 어렵게 만든다고 생각이 들어요. 만약 이동/비이동을 가진 numbers가 달라졌을때에는 기존의 테스트에도 영향을 미치구요.

sequence를 쓰지 않고 car의 position을 다르게 줄 수 있는 방법들이 몇가지 있어보여요. (Mock이 필요하지 않을수도 있겠네요) 한번 고민해보시겠어요?

Copy link

@pkch93 pkch93 left a comment

Choose a reason for hiding this comment

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

후니 피드백 반영한다고 고생했습니다..!
질문에 대한 답변과 추가 피드백도 드렸는데요. 이 부분은 2단계에서 부탁드릴께요.
궁금한 점 있으면 이 PR이 머지되더라도 댓글이나 DM 주세요.
2단계에서 뵐께요 :)

StringBuilder stringBuilder = new StringBuilder();
stringBuilder.append(this.name)
.append(NAME_POSITION_DELIMITER);
stringBuilder.append(PROGRESS_BAR.repeat(position));
Copy link

Choose a reason for hiding this comment

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

후니가 이번 자동차 경주 게임을 MVC 패턴을 적용했는데요. 지금 toString을 통해서 UI 부분을 구현하고 있네요. 모델 부분에 있는게 적절한지, 만약 적절하지 않다면 어떻게 개선하면 좋을지 고민하고 개선해보면 좋을거 같아요. :)

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 아무래도 View 가 있는 곳으로 빼는게 좋겠네요!!

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

2 participants