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

Step4 #3768

Open
wants to merge 20 commits into
base: boy0516
Choose a base branch
from
Open

Step4 #3768

wants to merge 20 commits into from

Conversation

boy0516
Copy link

@boy0516 boy0516 commented Apr 6, 2024

안녕하세요!

4단계 리뷰 부탁드립니다.
에러 처리와 재입력 로직을 추가했습니다.

작성을 하다보니 LottoController의 로직이 너무 길어지는거같습니다.
어떻게 코드를 정리해야하나요?

@boy0516 boy0516 changed the base branch from master to boy0516 April 6, 2024 13:05
Copy link

@wooobo wooobo left a comment

Choose a reason for hiding this comment

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

안녕하세요 지훈님!

4단계 잘진행해주셨습니다~
코멘트 남겨 놓았어요,
확인 후 재요청 부탁드려요~

Lottos lottos = new Lottos(getBudget());
ResultView.showGeneratedLottos(lottos);
private Lottos getLottos(Budget budget) {
SelfIssueCount count = getManualIssueCount();
Copy link

Choose a reason for hiding this comment

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

SelfIssueCount 일반적으로 이해하기 쉬운 표현인가요?
Count 그냥 카운트 아닌가요?

Count 수동금액갯수
불리어지는 곳에 따라 이름은 달라질 수 있지 않을까요?

}

public boolean isEnoughToPay(Price price) {
return budget > price.getValue();
Copy link

Choose a reason for hiding this comment

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

Suggested change
return budget > price.getValue();
return price.이것보다_작아?(price);

값을 꺼내기 보다는 상태를 가진 객체에게 물어볼수 있지 않을까요?

@@ -49,4 +60,8 @@ public boolean equals(Object o) {
public int hashCode() {
return Objects.hash(lotto);
}

protected boolean isContainBonus(LottoNumber bonus) {
Copy link

Choose a reason for hiding this comment

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

equals, hashCode 보다는 위에 있는게 일반적인것 같습니다. 🤔

@@ -29,6 +37,10 @@ public List<Lotto> getLottos() {
return lottos;
}

public int getLottoSize() {
Copy link

Choose a reason for hiding this comment

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

Suggested change
public int getLottoSize() {
public int size() {

어쩌면 우리는 무의식적으로 get-을 붙이고 있는게 아닐까요?
일급콜렉션은 하나의 상태를 관리하는 객체이므로 size() 라고 해도 충분한 의미전달이 된다고 생각해요 😄

@@ -4,63 +4,46 @@
import java.util.Objects;
import java.util.stream.Collectors;

public class WinLotto {
public class WinLotto extends Lotto {
Copy link

Choose a reason for hiding this comment

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

합성 보다 상속으로 구현하신 이유가 궁금해요! 🤔
개인적으론 합성이 더 적합하기도 하고 쉽지 않을까 생각이 되어서요!

[OOP] 코드의 재사용, 상속(Inheritance)보다 합성(Composition)을 사용해야 하는 이유

Comment on lines +22 to +25
} catch (Exception e) {
showErrorMessage(e);
return retryableInput(supplier);
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
} catch (Exception e) {
showErrorMessage(e);
return retryableInput(supplier);
}
}

Exception catch를 하는 이유가 궁금해요

Copy link
Author

Choose a reason for hiding this comment

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

파싱할 때 잘못된 폼으로 입력하면 오류가 발생하는데 임시로 처리해둔다는걸 그대로 둬버렸네요..

@@ -12,7 +12,7 @@
- [x] 로또 래퍼 클래스 구현
Copy link

Choose a reason for hiding this comment

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

스크린샷 2024-04-06 오후 11 41 00

결제 금액보다 많이 살수 없지 않을까요~?

Copy link

@wooobo wooobo left a comment

Choose a reason for hiding this comment

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

안녕하세요 지훈님!

코멘트 남겨 놓았어요~
확인 후 재요청 부탁드려요~

@@ -4,6 +4,8 @@
import java.util.List;
import java.util.stream.Collectors;

import static lotto.domain.Price.LOTTO;

public class Lottos {

private List<Lotto> lottos;
Copy link

Choose a reason for hiding this comment

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

Suggested change
private List<Lotto> lottos;
private final List<Lotto> lottos;

재할당을 하지말라고 final을 붙여주는건 어떨까요?

lottos.add(new Lotto());
return lottos;
}
return lottos;
Copy link

Choose a reason for hiding this comment

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

Collections.unmodifiableList

리턴해준 상태를 수정 불가로 해주면 어떨까요?

@@ -0,0 +1,32 @@
package lotto.domain;

public enum Price {
Copy link

Choose a reason for hiding this comment

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

enum을 사용한 이유가 궁금해요 🤔

Comment on lines +45 to 46
Lottos lottos = new Lottos(budget);
return lottos;
Copy link

Choose a reason for hiding this comment

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

Suggested change
Lottos lottos = new Lottos(budget);
return lottos;
return new Lottos(budget);

불필요한 변수 선언은 하지 않는게 복잡도를 덜어낼수 있을것 같아요~

Comment on lines +26 to +27
Lottos manualLottos = getManualIssuedLottos(manualIssueCount);
Lottos autoLottos = getAutoIssuedLottos(budget);
Copy link

Choose a reason for hiding this comment

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

Suggested change
Lottos manualLottos = getManualIssuedLottos(manualIssueCount);
Lottos autoLottos = getAutoIssuedLottos(budget);
Lottos manualLottos = InputView.inputSelfIssueLottos(manualIssueCount);
Lottos autoLottos = new Lottos(budget);

표현하는게 한줄인데
한번 더 메소드로 감싼 이유가 궁금해요 🤔
이것때문에 복잡해진다고 느껴져서요 🤔

Copy link

Choose a reason for hiding this comment

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

분리된 표현으로인해 복잡도가 올라갔고 그에 대해 전체적인 흐름의 그림이 잘 그려지지 않는다면,
다시 절차지향적으로 모아 보는것도 하나의 방법이 될 수 있을것 같아요.

스크린샷 2024-04-08 오후 9 52 24

저렇게 절차적으로 표현하고 나니,
저는 ResultView 의 기준으로 나누는게 크게 3분류로 나누어 보는게 어떨까? 라는 관점이 보이는것 같아요.

메소드가 분리된다면, 여기가 좀 애매해지네요.

        ResultView.showLottoQuantity(manualLottos, autoLottos);

manualLottos, autoLottos을 관리하는 객체가 하나로 나온다면 어떻게 변할지 궁금해지네요 🤔

    // Result 이름은 그냥 대충 지은것입니다.
    Result result = buyLotto();

이후 로직은 result 객체로 코드간의 대화를 이어가면 어떨까요?

Copy link

Choose a reason for hiding this comment

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

위는 저의 관점일 뿐입니다.

어떻게 코드를 정리해야하나요?

최고의 정리방법은 구현하면서 느꼈던 점을 상기시키면서 다시 구현하는거라고 개인적으론 생각합니다 🤔
물론 그과정은 지루하고 괴롭고 번거롭겠지만요. 😂

Copy link
Author

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.

2 participants