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단계 - 문자열 덧셈 계산기 #4171

Merged
merged 6 commits into from Apr 6, 2023
Merged

Conversation

dkswnkk
Copy link

@dkswnkk dkswnkk commented Apr 4, 2023

안녕하세요 멘토님!@ 잘부탁드립니다😀

return 0;
}

if (isContainsNegativeNumber(text)) {
Copy link
Author

@dkswnkk dkswnkk Apr 4, 2023

Choose a reason for hiding this comment

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

현재 아래와 같은 메서드를 구현한 뒤 해당 메서드를 통해 검증한 뒤 '-'를 포함하고 있다면, 검증 메서드 사용 로직에서 예외를 던지는 방법으로 구성했습니다.

    private static boolean isContainsNegativeNumber(String text) {
        return text.contains("-");
    }

하지만 현재 주어진 조건은 예외만 던지는 것이고 따로 분기하는 조건이 없기 때문에 아래와 같이 검증 메서드에서 바로 예외를 던지도록 바로 구현하는 방법도 괜찮을 것 같은데 멘토님의 의견이 궁금합니다!

    private static void validateContainsNegativeNumberAndThrow(String text) {
        throw new RuntimeException("Contains a negative number.");
    }

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.

예외를 검증 메소드를 사용하는 쪽에서 던지도록 한다면 예외 내용이 변경될 시 사용하는 모든 곳을 찾아다니며 변경해줘야 하기 때문이예요.

이렇게 부가적인 이유는 생각하지 못했었는데 감사합니다!!😯

Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

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

2단계 문자열 덧셈 계산기 미션 잘해주셨네요. 👍 👍

몇 가지 소소한 리뷰 남겨뒀으니 확인 한 번 부탁드릴게요.
그리고 제 의견 물어보신 부분도 대답 남겨뒀습니다.

그럼 화이팅입니당!

return 0;
}

if (isContainsNegativeNumber(text)) {

Choose a reason for hiding this comment

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

검증 메소드에서 바로 예외를 던지는걸 개인적으로 선호하는데요.
왜냐하면, 예외를 검증 메소드를 사용하는 쪽에서 던지도록 한다면 예외 내용이 변경될 시 사용하는 모든 곳을 찾아다니며 변경해줘야 하기 때문이예요.

private static final String DEFAULT_SEPARATOR = ",|:";
private static final String CUSTOM_SEPARATOR = "//(.)\n(.*)";

public static int splitAndSum(String text) {

Choose a reason for hiding this comment

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

문자열 덧셈 계산기는 덧셈책임만 가지도록 하고, split하는 책임은 별도의 객체를 만들어 위임해 보는 건 어떨까요? :)
(로직이 복잡하지 않아서, 새로운 객체를 만들었을 때의 이점을 크게 느끼진 못하겠지만 연습삼아 해보시면 좋을 것 같아요.)

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분은 작성하면서도 계속해서 나눌까 고민했던 부분인데 지적해 주셔서 감사합니다!!😀😀

Copy link

@serverwizard serverwizard left a comment

Choose a reason for hiding this comment

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

2단계 문자열 덧셈 계산기 잘 구현해 주셨네요 👍
머지하도록 하겠습니다.

추가로 소소한 리뷰 몇 가지 남겨뒀는데,
다음 미션하기 전에 참고하시면 좋을 것 같아요.

그럼 화이팅입니다.

}

private static int sum(String text) {
private static String[] splitNumberBySeparator(String text) {

Choose a reason for hiding this comment

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

책임 분리 👍
한 단계 더 나아가 해당 책임을 가진 Splitter 객체를 새롭게 만들어 보는 것도 좋을 것 같아요 :)

}

private static int sum(String text) {
private static String[] splitNumberBySeparator(String text) {
Matcher customMatcher = Pattern.compile(CUSTOM_SEPARATOR).matcher(text);

Choose a reason for hiding this comment

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

참고로, Pattern 객체는 생성 비용이 비싼 객체인데요.
이런 객체가 자주 사용되는 경우라면, 사용될 때마다 생성하는 방식보단 캐싱해서 사용하는 것이 좋을 것 같아요.
이를 위해 Pattern 객체를 static final로 선언하는 건 어떨까요?
image

@serverwizard serverwizard merged commit 4da6c0b into next-step:dkswnkk Apr 6, 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.

None yet

2 participants