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

[2단계 - 로또(수동)] 레넌(조형래) 미션 제출합니다. #473

Merged
merged 15 commits into from
Mar 7, 2022

Conversation

brorae
Copy link

@brorae brorae commented Mar 3, 2022

안녕하세요, 에단!
2단계도 잘 부탁드리겠습니다!

Copy link

@Laterality Laterality left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌,

전반적으로 잘 작성해주셨어요 👍

몇군데 코멘트를 남겨드렸으니 확인 부탁드려요!

@@ -1,40 +1,52 @@
package lotto.model;

import java.util.Map;
import lotto.model.generator.LottoGenerator;
import lotto.model.number.LottoNumber;
import lotto.model.number.LottoNumbers;

public class LottoMachine {

Choose a reason for hiding this comment

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

LottoMachine이라는 이름이 현실 세계에서 로똔 복권을 구입하는 기계를 연상시키는데요. 실제 기계와는 달리 이 클래스는 로또를 한번 구입하면 (새 복권을 구입할 수 없고)수명이 다하는 걸로 보입니다. 실제 기계처럼 로또를 여러번 구입할 수 있도록 만들어보면 어떨까요?

Choose a reason for hiding this comment

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

의도가 조금 잘못 전달됐네요 😅

LottoMachineLottos, Money, RankCount와 같이 로또 게임이 한 번 끝나면 필요 없는 객체들을 필드로 갖는데요. 이런 필드 없이 처리할 수 있다면 여러번의 게임에서 재사용할 수 있지 않을까 싶어 드린 의견이었습니다.

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

맞습니다. LottoMachineMoney, LottoCount, Lottos, RankCount와 같은 필드는 로또를 한 번 구입하는데 필요한 클래스들인데요. LottoMachine에서는 이 인스턴스들을 생성자에서 만들고 있기 때문에 하나의 LottoMachine 인스턴스로는 여러 번의 로또 구입을 처리할 수 없습니다.

(줄서서 기다려야 하겠지만)여러 사람이 하나의 로또 기계에서 로또를 구입할 수 있는 경우에 빗댈 수 있겠네요.

Copy link
Author

Choose a reason for hiding this comment

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

LottoMachine에서 필드를 없애는 대신, 이후에 구매가 가능하도록 필드를 두고 추가하는 방식으로 작성하였는데, 이 방식도 괜찮은 방식일까요??

Choose a reason for hiding this comment

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

현재 요구사항에서는 충분합니다 👍

여유가 되신다면 필드 없이 구현할 수 있는 방법에 대해서도 고민해보시면 좋은 인사이트를 얻으실 거라 생각해요.

src/main/java/lotto/model/Lottos.java Outdated Show resolved Hide resolved
src/main/java/lotto/model/Money.java Outdated Show resolved Hide resolved
src/main/java/lotto/model/Money.java Outdated Show resolved Hide resolved
src/main/java/lotto/model/RankCount.java Outdated Show resolved Hide resolved
Copy link

@Laterality Laterality left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌,

잘 고쳐주셨어요 👍

컨트롤러가 필요 이상으로 커져있는 걸로 보여요.

컨트롤러에 너무 많은 책임이 있는 건 아닐지 고민해보시면 좋겠습니다.

함께 남겨드린 피드백도 확인 부탁드릴게요.

src/main/java/lotto/model/RankCount.java Outdated Show resolved Hide resolved
src/main/java/lotto/model/Money.java Outdated Show resolved Hide resolved
src/main/java/lotto/model/Lottos.java Outdated Show resolved Hide resolved
src/main/java/lotto/controller/LottoController.java Outdated Show resolved Hide resolved
src/main/java/lotto/controller/LottoController.java Outdated Show resolved Hide resolved
src/main/java/lotto/controller/LottoController.java Outdated Show resolved Hide resolved
@@ -1,40 +1,52 @@
package lotto.model;

import java.util.Map;
import lotto.model.generator.LottoGenerator;
import lotto.model.number.LottoNumber;
import lotto.model.number.LottoNumbers;

public class LottoMachine {

Choose a reason for hiding this comment

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

의도가 조금 잘못 전달됐네요 😅

LottoMachineLottos, Money, RankCount와 같이 로또 게임이 한 번 끝나면 필요 없는 객체들을 필드로 갖는데요. 이런 필드 없이 처리할 수 있다면 여러번의 게임에서 재사용할 수 있지 않을까 싶어 드린 의견이었습니다.

src/main/java/lotto/model/LottoCount.java Outdated Show resolved Hide resolved
Copy link

@Laterality Laterality left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌,

컨트롤러가 훨씬 간결해졌어요 💯

몇가지 추가로 고민해볼 만한 곳에 코멘트를 남겨드렸는데, 확인 부탁드릴게요 :)

@@ -1,40 +1,52 @@
package lotto.model;

import java.util.Map;
import lotto.model.generator.LottoGenerator;
import lotto.model.number.LottoNumber;
import lotto.model.number.LottoNumbers;

public class LottoMachine {

Choose a reason for hiding this comment

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

맞습니다. LottoMachineMoney, LottoCount, Lottos, RankCount와 같은 필드는 로또를 한 번 구입하는데 필요한 클래스들인데요. LottoMachine에서는 이 인스턴스들을 생성자에서 만들고 있기 때문에 하나의 LottoMachine 인스턴스로는 여러 번의 로또 구입을 처리할 수 없습니다.

(줄서서 기다려야 하겠지만)여러 사람이 하나의 로또 기계에서 로또를 구입할 수 있는 경우에 빗댈 수 있겠네요.

src/main/java/lotto/controller/LottoController.java Outdated Show resolved Hide resolved
src/main/java/lotto/controller/LottoController.java Outdated Show resolved Hide resolved
src/main/java/lotto/model/RankCount.java Show resolved Hide resolved
src/main/java/lotto/model/RankCount.java Outdated Show resolved Hide resolved
@@ -28,20 +28,26 @@ void runLottoControllerTest() {

public void run() {

Choose a reason for hiding this comment

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

컨트롤러의 run 메서드 구현에 테스트용 데이터로 치환된 메서드를 테스트하고 있는데요.

테스트 해야 하는 대상은 컨트롤러인데, 정작 테스트하고 있는 것은 테스트 클래스에 똑같이 구현된 run 메서드입니다.

이 테스트는 테스트로서 어떤 의미가 있을까요?

Copy link
Author

@brorae brorae Mar 6, 2022

Choose a reason for hiding this comment

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

Scanner로 받아오는 test를 하기 어렵다고 생각해서, 구현 로직이 똑같은 메소드를 Test에 만들어서 Scanner 대신 임의의 값으로 대입하여 테스트를 해주었습니다.

Input을 테스트 하는 방법을 공부하여 테스트 코드를 작성해보았습니다.

Domain(model)만 지금 껏 테스트를 해왔는데, Controller에서도 테스트를 무조건 진행해야 하는 것인지 궁금합니다.

Choose a reason for hiding this comment

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

단위 테스트는 가능한 모든 코드에 작성하는 것이 좋습니다.

테스트 커버리지라고 부르는데, 프로덕션 코드에서 테스트가 이루어지는 코드의 비율을 코드 퀄리티의 지표 중 하나로 삼기도 합니다.

오히려 테스트하는 듯하면서 아무것도 테스트하지 않는 테스트는 읽는 사람에게 혼란을 줄 수 있어요.

Scanner로 받아오는 부분 때문에 컨트롤러를 테스트하기 어렵다고 하셨는데요. 어떻게 하면 컨트롤러를 테스트할 수 있게 만들 수 있을까요? 아마 답은 이미 찾으신 것 같은데요 😏

Copy link

@Laterality Laterality left a comment

Choose a reason for hiding this comment

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

안녕하세요 레넌,

깔끔하네요 💯

추가로 피드백할 만한 내용은 없어서 이만 머지하도록 할게요.

다음 미션 전까지 여유가 되신다면 컨트롤러 테스트에 관해 고민해보시면 도움이 될 거라 생각해요.

추가로 궁금하신 점이나 질문 있으시면 언제든 DM 주시고요 :)

@@ -28,20 +28,26 @@ void runLottoControllerTest() {

public void run() {

Choose a reason for hiding this comment

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

단위 테스트는 가능한 모든 코드에 작성하는 것이 좋습니다.

테스트 커버리지라고 부르는데, 프로덕션 코드에서 테스트가 이루어지는 코드의 비율을 코드 퀄리티의 지표 중 하나로 삼기도 합니다.

오히려 테스트하는 듯하면서 아무것도 테스트하지 않는 테스트는 읽는 사람에게 혼란을 줄 수 있어요.

Scanner로 받아오는 부분 때문에 컨트롤러를 테스트하기 어렵다고 하셨는데요. 어떻게 하면 컨트롤러를 테스트할 수 있게 만들 수 있을까요? 아마 답은 이미 찾으신 것 같은데요 😏

@@ -1,40 +1,52 @@
package lotto.model;

import java.util.Map;
import lotto.model.generator.LottoGenerator;
import lotto.model.number.LottoNumber;
import lotto.model.number.LottoNumbers;

public class LottoMachine {

Choose a reason for hiding this comment

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

현재 요구사항에서는 충분합니다 👍

여유가 되신다면 필드 없이 구현할 수 있는 방법에 대해서도 고민해보시면 좋은 인사이트를 얻으실 거라 생각해요.

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