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

Merged
merged 15 commits into from
Mar 14, 2025

Conversation

HongYeseul
Copy link

@HongYeseul HongYeseul commented Mar 11, 2025

2단계 - 문자열 덧셈 계산기

  • 계산하는 기능을 하는 Calculator 클래스와 문자열 파싱과 관련된 기능을 하는 StringParser 클래스로 나누어 보았습니다.
  • 기본 구분자로 동작하는 ,:를 String 배열에 저장해 두고, 배열들을 한 번에 join해서 관리할 수 있도록 상수화 했습니다.

라이브 수업을 들으면서 알게된 부분들이 있어 메서드를 나누며 수정했습니다.
수업을 들으며 ... 최대한 간단하게 만들 수 있는 기능은 간단하게 접근하자고 느꼈습니다.

참고


오늘도 잘 부탁드립니다 👍
좋은 하루 되세요!

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

조금 더 개선해보면 좋을 부분에 리뷰를 남겨두었으니 확인해주세요.

모든 리뷰를 반드시 반영해야하는 것은 아니나 납득이 되지 않는 부분, 이해가 가지 않는 부분, 어려운 부분은 가감없이 코멘트를 달아주세요 :)

Comment on lines 21 to 27
if (StringParser.hasCustomDelimiter(input)) {
return sumStringArray(StringParser.parseWithCustomDelimiterOrNull(input));
}

if (hasDefaultDelimiter(input)) {
return sumStringArray(input.split(DEFAULT_DELIMITER_REGEX));
}
Copy link

Choose a reason for hiding this comment

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

요구사항을 충족하도록 작성해주셨네요~!

한가지 조금 아쉬운점이 있는것 같은데요

구분자의 개수가 늘어나면 여기에 if절이 계속해서 늘어나게 되는걸까요?

이를 해결하기 위해 구분자를 멤버변수로 가지고 있고 문자열을 구분할 수 있는 역할을 가지는 객체를 설계해보면 어떨까요?

Comment on lines 35 to 41
private static boolean isValidInput(String input) {
if (input == null || input.isBlank()) {
return false;
}
StringParser.hasNegativeNumber(input);
return true;
}
Copy link

Choose a reason for hiding this comment

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

이 부분도 하나의 객체에 역할을 할당해볼 수 있을것 같은데요!

class PositiveNumber 라는 클래스가 있다면 이러한 역할을 해줄수 있지 않을까요?

Copy link

Choose a reason for hiding this comment

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

이러한 방식으로 특정 역할을 하는 객체에게 멤버변수를 할당해서 역할을 부여해보시면 좋을것 같습니다 :)

* @return 계산 결과
*/
public static int splitAndSum(String input) {
if (!isValidInput(input)) {
Copy link

Choose a reason for hiding this comment

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

!를 이용해 부정 표현을 하는것도 좋은 방법중 하나인데요!

코드를 읽을때 인지요소를 낮추면 코드를 읽는 사람이 더 편해지기 마련이기에 처음부터 부정 표현으로 메서드 네이밍을 해보는건 어떨까요?

ex.
is -> isNot
has -> hasNot

return getCustomMatcher(input).matches();
}

public static String[] parseWithCustomDelimiterOrNull(String input) {
Copy link

Choose a reason for hiding this comment

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

List를 사용하는것도 하나의 방법이 될 수 있을것 같은데요! 혹시 배열을 사용하신데 이유가 있을까요?

Copy link
Author

@HongYeseul HongYeseul Mar 12, 2025

Choose a reason for hiding this comment

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

안녕하세요!
리스트를 사용할 수도 있지만, 현재 Matcher가 반환하는 값이 String[]이라 배열을 그대로 사용하는 게 더 적절하다고 생각했습니다.

일반 배열은 리스트보다 메모리를 적게 사용하고, 순차 접근이 필요할 때 더 효율적일 수 있기 때문입니다.
다만, 가독성 면에서는 리스트가 더 나을 수도 있지만, 현재 요구사항이 단순한 계산기인 만큼 굳이 리스트로 변경해야 할까? 라는 고민이 드네요.

어떻게 생각하시나요?
(이번 리팩토링에서는 리스트로 변경하는 방향으로 진행해 보려고 합니다.)

Copy link

Choose a reason for hiding this comment

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

말씀하신 것 처럼 지금 요구사항이 단순한 만큼 배열을 쓰는것과 리스트를 쓰는것의 큰 차이는 없을것 같습니다~

리팩토링은 원하는 방향으로 해보셔도 좋습니다 :)

Comment on lines 30 to 31
final String delimiter = customMatcher.group(1);
return customMatcher.group(2).split(delimiter);
Copy link

Choose a reason for hiding this comment

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

1과 2가 어떠한 의미인지 명확히 나타낼 수 있다면 더욱 코드를 읽기 수월할 것 같습니다. :)

https://bottom-to-top.tistory.com/100

@@ -0,0 +1,43 @@
package step2.util;
Copy link

Choose a reason for hiding this comment

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

이번 미션을 비롯하여 util성 클래스는 앞으로 미션을 진행하면서 사용을 하지 않고 구현해보면 좋을것 같습니다.

지금 문자열 덧셈 계산기에 있어서 StringParser의 역할을 유틸성 기능이 아니라 요구사항을 해결하는데 있어 매우 핵심로직들을 담고 있는데 util이라는 이름을 붙이기에는 조금 아까운것 같네요 :)

Matcher negativeMatcher = getNegativeMatcher(input);

if (negativeMatcher.matches()) {
throw new RuntimeException("음수는 사용할 수 없습니다.");
Copy link

Choose a reason for hiding this comment

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

exception 메세지에는 디버그를 하기 쉽도록 원인이 된 값을 함께 포함해주면 더욱 좋습니다. :)

Comment on lines 14 to 22
@Test
@DisplayName("null 또는 빈문자를 입력할 경우 0을 반환해야 한다.")
public void splitAndSum_null_또는_빈문자() {
int result = StringAddCalculator.splitAndSum(null);
assertThat(result).isEqualTo(0);

result = StringAddCalculator.splitAndSum("");
assertThat(result).isEqualTo(0);
}
Copy link

Choose a reason for hiding this comment

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

이러한 경우 @NullAndEmptySource를 이용해서 표현해보면 더욱 좋을것 같습니다~

Comment on lines 46 to 50
@DisplayName("구분자는 //와 /n 문자를 이용해 커스텀 구분자를 지정할 수 있다.")
public void splitAndSum_custom_구분자() throws Exception {
int result = StringAddCalculator.splitAndSum("//;\n1;2;3");
assertThat(result).isEqualTo(6);
}
Copy link

Choose a reason for hiding this comment

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

테스트의 이름은 커스텀 구분자가 동작한다는 것을 보고 싶은것 같습니다.

splitAndSum 내에서 이러한 동작이 올바르게 된다는 보장을 할 수 있을까요?

가령 문자열에서 숫자만 파싱해서 더하고있다면 이는 버그지만 테스트에서 보장해주지 못하는 상황이지 않을까요?

테스트에서 검증하고자 하는대상을 보다 확실하게 검증할 수 있게할수 있는 방향을 한번 고려해보면 좋을것 같습니다. :)

* @param input 입력값
* @return 계산 결과
*/
public static int splitAndSum(String input) {
Copy link

Choose a reason for hiding this comment

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

splitAndSum이라는 이름에서 두가지 책임이 보이는것 같습니다!

SRP 법칙에 따라 하나의 변경사항에 대해서만 코드가 변경될 수 있도록 내부 구현의 책임을 서로다른 객체에게 나누어주면 좋을것 같습니다.

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

Choose a reason for hiding this comment

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

네 좋습니다 :)

Copy link
Author

@HongYeseul HongYeseul left a comment

Choose a reason for hiding this comment

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

@pci2676
리뷰 해주신 부분들 중 리팩토링 해서 푸시 해두었습니다.

splitAndSum() 메서드와 관련된 부분은 1주차 라이브 수업 때 들었던 내용을 바탕으로 수정할 수 있을 것 같아 보입니다.
오늘 내로 수정하겠습니다 ..!

Copy link

@pci2676 pci2676 left a comment

Choose a reason for hiding this comment

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

다음단계를 진행하며 이번단계에 남겨진 리뷰를 참고해보세요~

merge하겠습니다~ 🚀


private static final Pattern CUSTOM_DELIMITER_PATTERN = Pattern.compile(CUSTOM_DELIMITER_REGEX);

public static Matcher getCustomMatcher(String input) {
Copy link

Choose a reason for hiding this comment

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

static function만 가지고 있는 클래스는 util성 클래스와 그 형태가 비슷해지는데요 :)

이 클래스를 Default delimiter도 사용할 수 있도록 클래스를 설계해보면 좋을것 같습니다.

@ParameterizedTest
@DisplayName("숫자 하나를 문자열로 입력할 경우 해당 숫자를 반환한다.")
@ValueSource(strings = {"1", "2", "3"})
public void calculate_숫자하나(String input) throws Exception {
Copy link

Choose a reason for hiding this comment

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

모든 테스트에서 throw를 하고 있는것 같은데 필요하지 않는것 같아보이네요~

Comment on lines +57 to +67
private static int toInt(String value) {
int number = Integer.parseInt(value);
checkNegativeNumber(number);
return number;
}

private static void checkNegativeNumber(int number) {
if (number < 0) {
throw new RuntimeException(String.format("음수(%s)는 사용할 수 없습니다.", number));
}
}
Copy link

Choose a reason for hiding this comment

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

이러한 역할을 갖는 클래스를 도출해보면 좋을것 같습니다 :)

@pci2676 pci2676 merged commit 2399c16 into next-step:hongyeseul Mar 14, 2025
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