-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Step4 : 로또(수동) #4226
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
base: mo-bile
Are you sure you want to change the base?
Step4 : 로또(수동) #4226
Conversation
…er의 생성자 생성을 막기위해 class 로 변경
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
객체가 잘 나누어져서인지 수동입력의 변경 여파가 로또 자동생성 및 당첨 계산, 수익률 계산에 코드수정이 거의 없어서 뿌듯했습니다. 이 부분을 제대로 느꼈군요. 👍
객체 설계를 잘 했을 때, 유지보수하기 좋은 코드를 구현했을 때 이 같은 느낌을 받을 수 있습니다.
객체의 의존관계를 끊고, 독립적으로 구현했을 경우 얻을 수 있는 효과입니다.
로또 생성과 관련해 다른 접근 방식으로 고민해 봤으면 하는 바람으로 피드백 남겨요.
피드백 반영하면서 시간날 때 '수강신청' 미션의 1단계 - 레거시 코드 리팩터링를 병행해 볼 것을 추천합니다.
|
안녕하세요? LottoBuy 의 생성자 부분을 분리해서 각 인터페이스의 메서드로 분리하였습니다. 감사합니다! |
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인터페이스 구현과 관련한 피드백 하나 남겼으니 도전해 보면 좋겠습니다.
로또 마지막 단계인만큼 이 연습을 하면서 "수강신청 - 레거시 코드 리팩터링" 미션의 "1단계 - 레거시 코드 리팩터링" 미션 병행할 것을 추천합니다.
미션을 병행하는 만큼 이 피드백 반영에 조급해 하지 말고 충분히 여유를 가지고 고민해 봤으면 좋겠습니다.
| LottoGenerator lottoGenerator = new LottoCombineGenerator(); | ||
| if(isAllAutoGenerate(manualCount)) { | ||
| lottoGenerator = new LottoAutoGenerator(); | ||
| } | ||
| if(isAllManualGenerate(manualCount, pay)) { | ||
| lottoGenerator = new LottoManulGenerator(); | ||
| } | ||
| List<String> manualLottoNumbers = readManualLottos(manualCount); | ||
| LottoBuy lottoBuy = lottoGenerator.generate(pay, manualLottoNumbers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인터페이스 기반으로 구현하면서 오히려 복잡도가 높아진 느낌이 든다.
인터페이스 기반으로 구현했을 때의 테스트 예시이다.
아래와 같이 구현하기 위해 도전해 보는 것은 어떨까?
아래 예시에서 LottoBundleGenerator는 LottoCombineGenerator와 같은 역할
Lottos는 LottoTickets와 같음
class LottoBundleGeneratorTest {
@Test
void generate() {
Money money = new Money(5_000);
List<String> values = List.of(
"1,2,3,4,5,6",
"2,3,4,5,6,7",
"3,4,5,6,7,8"
);
LottoGenerator lottoGenerator = new LottoBundleGenerator(money, values);
Lottos lottos = lottoGenerator.generate();
assertThat(lottos.size()).isEqualTo(money.countOfBuyingLotto());
System.out.println(lottos);
}
}|
안녕하세요 인터페이스에 대한 부족한 이해로 인터페이스 분리 방식이 복잡성을 가중시켰던 것 같습니다 고민이 있습니다. |
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
질문과 관련한 내용은 코드 리뷰 피드백을 통해 고민해 봤으면 합니다.
LottoGenerator가 로또를 생성하는 것이 아니라 LottoBuy(수동/자동과 관련있는)를 생성하다보니 오히려 복잡도가 높아지는 것 같아요.
LottoGenerator가 로또의 목록을 생성하도록 구현한다면 훨씬 더 단순하게 구현할 수 있지 않을까요?
|
|
||
| public interface LottoGenerator { | ||
|
|
||
| LottoBuy generate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| LottoBuy generate(); | |
| LottoTickets generate(); |
LottoBuy를 생성해 반환하다보니 Auto인 경우에도 Manual, Manual인 경우에도 Auto 값을 가짐
각각의 Generator 구현체에서 바로 LottoTickets를 생성하도록 접근해 본다.
현재 구현과 어떻게 다른지 고민해 본다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LottoBuy 로 생성 후 반환하려고 하다 보니 LottoBuy의 생성자 틀에 억지로 맞추어야했다.
그래서 Manual 인 경우에도 Auto 값을 가지게 되었다
그에 맞춰 각각 구현체의 필드도 모두 같아야한다고 생각하게 되었다.
|
|
||
| public class LottoManulGenerator implements LottoGenerator { | ||
|
|
||
| private final int pay; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pay는 몰라도 되지 않을까?
manualLottoNumbers 값만 있으면 수동 로또 생성이 가능하다.
|
안녕하세요? 피드백 요청드립니다! 우선 감사드립니다 👍 현재 상태가 복잡하다면, 여러 객체 중 이 부분이 꼭 필요할까 곧바로 가능하지 않을까? 이런 고민도 함께 하겠습니다 모호한 인터페이스에 대한 이해가 점점 잡혀갑니다. 💯 |
javajigi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아마도 로또의 마지막 피드백이 될 것 같네요.
LottoAutoGenerator, LottoManualGenerator와 같은 객체가 이미 있는데 Auto, Manual과 같은 객체는 없어도 되지 않을까라는 생각이 들어 피드백 남겨 봅니다.
각각의 객체의 역할과 책임에 대해 고민해 보면 어떨까요?
| import java.util.stream.Stream; | ||
| import lotto.domain.model.*; | ||
|
|
||
| public class LottoCombineGenerator implements LottoGenerator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LottoAutoGenerator, LottoManualGenerator와 같은 객체가 있는데 굳이 Auto, Manual과 같은 객체는 없어도 되지 않을까?
아래 코드의 Lottos는 재영님 코드의 LottoTickets와 같은 역할을 함
이와 같이 인터페이스를 구현하고, 구현체가 서로 연결되어 있는 경우 아래와 같은 구현체를 통해 구현체를 통합할 수 있음.
보통 디자인 패턴에서 컴포지트 패턴으로 알려져 있음.
아래와 같은 객체 추가에 따른 효과를 느껴 봤으면 하는 바람으로 피드백 남겨봄.
public class LottosBundleGenerator implements LottosGenerator {
private final List<LottosGenerator> lottosGenerators;
public LottosBundleGenerator(Money money, List<String> manualLottoText) {
this(toLottosGenerators(money, manualLottoText));
}
private static List<LottosGenerator> toLottosGenerators(Money money, List<String> manualLottoText) {
// manualLottoText 활용해 ManualLottosGenerator 생성
// money에서 수동으로 구매한 로또 수 만큼 차감한 후 AutoLottosGenerator 생성
}
public LottosBundleGenerator(List<LottosGenerator> lottosGenerators) {
this.lottosGenerators = lottosGenerators;
}
@Override
public Lottos generate() {
// List<LottosGenerator> 반복문 돌며 로또 생성
return 두 로또를 합쳐서 반환;
}
}
안녕하세요?
피드백 요청드립니다 👍
객체가 잘 나누어져서인지 수동입력의 변경 여파가 로또 자동생성 및 당첨 계산, 수익률 계산에 코드수정이 거의 없어서 뿌듯했습니다.
크게는 LottoGame 외 별도의 LottoBuy라는 흐름을 새롭게 만들었는데 이렇게 로또 입력과, 계산 크게 두개 비즈니스 흐름을 만들었습니다.
이렇게 접근해도 되는건지 고민을 하다가 시도했습니다.
잘 부탁드리겠습니다.
감사합니다 👍