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주차] 변해빈 #1

Open
wants to merge 63 commits into
base: main
Choose a base branch
from
Open

[1주차] 변해빈 #1

wants to merge 63 commits into from

Conversation

h-beeen
Copy link
Member

@h-beeen h-beeen commented Oct 22, 2023

README.md 도 있어요!

Comment on lines +49 to +53
private List<Integer> convertInputNumber(String input) {
return input.chars()
.mapToObj(Character::getNumericValue)
.toList();
}

Choose a reason for hiding this comment

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

이런 식으로 문자열을 리스트로 바꾸는 거는 생각을 못 했는데 간결하고 좋은 것 같습니다👍

Comment on lines 23 to 34
private ResultType inspectResultType() {
if (!hasBall() && !hasStrike()) {
return NOTHING;
} else if (hasBall() && hasStrike()) {
return BALL_AND_STRIKE;
} else if (hasBall()) {
return ONLY_BALL;
} else if (hasStrike()) {
return ONLY_STRIKE;
}
throw new IllegalArgumentException("result type error");
}

Choose a reason for hiding this comment

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

지금도 간결한 구조이긴 한데 else if는 빼고 if만 4번 넣어도 될 것 같습니당
어디서 자꾸 else if 쓰지 말라고 들었는데 이렇게 하는 게 맞는진 모르겠네요..ㅋㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

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

껄껄 else 없는 삶..서럽

Comment on lines +3 to +19
public enum ResultType {

NOTHING("낫싱"),
ONLY_BALL("%d볼"),
ONLY_STRIKE("%d스트라이크"),
BALL_AND_STRIKE("%d볼 %d스트라이크");

private final String value;

ResultType(String value) {
this.value = value;
}

public String getValue() {
return value;
}
}

Choose a reason for hiding this comment

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

Enum 타입은 쓸 생각을 못 했는데 저렇게 %d까지 넣어서 처리하는 건 좀 sexy한 것 같습니다👍

Comment on lines +17 to +24
private Number(String input) {
validateEmpty(input);
validateNumberLength(input);
validateContainOnlyNumber(input);
validateContainDuplicatedNumber(input);

this.numbers = convertInputNumber(input);
}

Choose a reason for hiding this comment

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

여기서 검증 메소드를 다 사용 안 한 이유가 있나요? 분명 7개 만든 것 같은데 4개 밖에 없어서ㅋㅋㅋ

Copy link
Member Author

@h-beeen h-beeen Oct 23, 2023

Choose a reason for hiding this comment

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

    public static void validateContainOnlyNumber(final String number) {
        if (!isValidNumber(number)) {
            throw new IllegalArgumentException("number cannot contain any letters");
        }
    }

isValid 메소드는 실제 검증을 수행한 결과를 boolean으로 리턴합니다.
그리고 이 메소드 호출 결과에 따른 포스트 컨디션으로
validate 메소드를 활용해는 검증 -> 예외를 던지는 역할로 분리했습니다.

따라서 상기 4개의 메소드를 호출하면
7개의 메소드를 전부 활용한 검증이 가능한거죠!

@holyPigeon
Copy link

ㅋㅋㅋ아니 리드미를 지금 봤네요 아.. 저거 빨리 봤으면 눈 좀 덜 아팠을텐데ㅋㅋㅋ
패키지 구조 저거는 진짜 좀 잘 만든 것 같아서 벤치마킹좀 하겠습니다ㅋㅋ 개굿이네염👍

Comment on lines 45 to 49
public void print() {
ResultType resultType = inspectResultType();
String message = generateResultMessage(resultType);
OutputView.printResult(message);
}

Choose a reason for hiding this comment

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

제 생각에 mvc 구조에서는 OutputView가 도메인 계층보다는 컨트롤러 계층에서 쓰이면 좋을 것 같아서 print() 메소드가 String을 반환하도록 한다음 컨트롤러에서 처리하면 더 좋을 것 같습니다!

Suggested change
public void print() {
ResultType resultType = inspectResultType();
String message = generateResultMessage(resultType);
OutputView.printResult(message);
}
public string getResultMessage() {
ResultType resultType = inspectResultType();
return generateResultMessage(resultType);
}
// 컨트롤러
OutputView.printResult(result.getResultMessage);

Copy link
Member Author

Choose a reason for hiding this comment

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

정말 좋은 의견인 것 같아요!
반영해볼게요!

@holyPigeon holyPigeon closed this Oct 23, 2023
@holyPigeon holyPigeon deleted the h-beeen branch October 23, 2023 13:28
@holyPigeon holyPigeon restored the h-beeen branch October 23, 2023 13:34
@holyPigeon
Copy link

푸시하다가 실수로 브랜치 지워서 PR이 닫혔네요...ㅠㅠ 죄송쓰

@h-beeen h-beeen reopened this Oct 23, 2023
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