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

Merged
merged 17 commits into from
Jan 27, 2025
Merged

Conversation

devFancy
Copy link

@devFancy devFancy commented Jan 25, 2025

안녕하세요. 리뷰어님!
이번 미션에서는 요구사항 충족에 집중하며 간결하고 직관적인 코드를 작성하고자 했습니다.
아래는 이번 구현의 주요 내용과 의도입니다.


구현 내용

클래스 설계

단일 책임 원칙(Single Responsibility Principle)을 고려했지만, 현재 요구사항에서는 지나치게 세분화하는 것보다 이해하기 쉽고 관리하기 용이한 구조가 더 적합하다고 판단했습니다.
따라서 다음과 같이 두 개의 클래스로 나누었습니다.

  • StringCalculator: 문자열 덧셈 계산을 담당하는 핵심 클래스입니다.
  • StringSplitter: 문자열을 특정 구분자로 분리하는 역할을 담당합니다.

요구사항

모든 요구사항을 충족하였으며, 세부 요구사항은 1단계_요구사항.md 파일에 정리했습니다.


구현한 코드에서 이해가 어려운 부분이나 부족한 점이 있다면 언제든 편하게 말씀해주세요!
리뷰 잘 부탁드립니다 ! 😊
감사합니다.

@devFancy devFancy changed the base branch from master to devfancy January 25, 2025 14:15
Copy link

@mj950425 mj950425 left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다 준용님 👍

단일 책임 원칙(Single Responsibility Principle)을 고려했지만, 현재 요구사항에서는 지나치게 세분화하는 것보다 이해하기 쉽고 관리하기 용이한 구조가 더 적합하다고 판단했습니다.

현재 요구사항이 복잡한것은 아니지만, 의식적으로 클린 코드를 연습하는 과정의 미션인만큼 말씀해주신 단일책임원칙에 맞게 코드를 작성해보는것도 좋아보입니다.

관련해서 짧게 코멘트 남겼습니다. 확인 부탁드려요 💪


public class StringAdditionCalculator {

private String text;

Choose a reason for hiding this comment

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

클래스 변수에도 final을 붙여주는것은 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분 수정했습니다!

return 0;
}

String[] numbers = StringSplitter.split(text);

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.

넵! 그래서 숫자를 검증하고 정수를 변환하는 역할을 PositiveNumber 클래스를 생성해서 책임을 위임했습니다!

Choose a reason for hiding this comment

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

String[] numbers = StringSplitter.split(text);

배열 대신 콜랙션을 사용해보는것은 어떨까요?

아니면 따로 배열을 사용하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

현재 코드에서 확장성을 고려하지 못한 것 같습니다.
컬렉션 사용하여 반영했습니다!

@@ -0,0 +1,11 @@
root = true

Choose a reason for hiding this comment

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

👍

Comment on lines +28 to +29
@ParameterizedTest
@NullAndEmptySource

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,24 @@
package calculator;

public class StringSplitter {

Choose a reason for hiding this comment

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

생성자의 접근 제한자를 private으로 설정하면 유틸리티 클래스가 인스턴스화되는 것을 방지할 수 있습니다.

Suggested change
public class StringSplitter {
private StringSplitter() {
}

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

@mj950425 mj950425 Jan 27, 2025

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.

아래와 같이 생성자를 private로 반영했어요 !

private StringSplitter() {
        throw new UnsupportedOperationException("StringSplitter is a utility class and cannot be instantiated.");
    }

Choose a reason for hiding this comment

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

아하 제가 코드 확인을 못한 것 같습니다. 잘 반영해주셨네요 👍

- private 설정함으로서 유틸리티 클래스가 인스턴스화 되는 것을 방지
- 유틸리티 클래스임을 명확히 표현하기 위해 생성자 호출시 예외 던지도록 변경
@devFancy devFancy changed the title Step1: 1단계 - 문자열 덧셈 계산기 1단계 - 문자열 덧셈 계산기 Jan 27, 2025
Copy link

@mj950425 mj950425 left a comment

Choose a reason for hiding this comment

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

빠른 리뷰 반영 고생하셨어요 💪

몇가지 코멘트만 추가적으로 남겨뒀는데 확인 부탁드릴게요.

String[] numbers = StringSplitter.split(text);
int sum = 0;

for (String number : numbers) {

Choose a reason for hiding this comment

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

일급 콜랙션을 사용해보는것은 어떨까요?

래퍼런스 : https://jojoldu.tistory.com/412

Copy link
Author

Choose a reason for hiding this comment

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

넵! 반영했습니다!

return 0;
}

String[] numbers = StringSplitter.split(text);

Choose a reason for hiding this comment

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

String[] numbers = StringSplitter.split(text);

배열 대신 콜랙션을 사용해보는것은 어떨까요?

아니면 따로 배열을 사용하신 이유가 있을까요?

@@ -0,0 +1,24 @@
package calculator;

public class StringSplitter {
Copy link

@mj950425 mj950425 Jan 27, 2025

Choose a reason for hiding this comment

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

반영이 안되어있네요!

@@ -0,0 +1,35 @@
package calculator;

import calculator.exception.InvalidNumberFormatException;

Choose a reason for hiding this comment

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

사용하지않는 Import 문은 제거해도될것같아요

Copy link
Author

Choose a reason for hiding this comment

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

반영했습니다!~

@devFancy devFancy requested a review from mj950425 January 27, 2025 07:26
- 숫자 검증 및 값을 반환은 `PositiveNumber`
- 여러 PositiveNumber 객체를 관리하고 합을 구하는 역할은 `PositiveNumbers`
Copy link

@mj950425 mj950425 left a comment

Choose a reason for hiding this comment

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

낮부터 고생하셨습니다 👍👍

마이너한 코멘트만 남겨두었고, 이번 스탭은 머지하도록 하겠습니다~

다음 미션으로 가시죠!

/**
* 숫자 검증 및 변환하는 역할
*/
public class PositiveNumber {

Choose a reason for hiding this comment

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

책임에 맞게 객체를 분리했으니, 테스트 코드도 분리할 수 있을 것 같아요.

/**
* 0 이상의 숫자들의 리스트를 관리하는 일급 컬렉션
*/
public class PositiveNumbers {

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.

이 부분은 2단계 미션 진행하면서 같이 짜보도록 할게요!

public PositiveNumbers(final List<String> numberToStrings) {
this.numbers = numberToStrings.stream()
.map(PositiveNumber::new)
.collect(Collectors.toList());

Choose a reason for hiding this comment

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

자바 21을 사용하고 계시네요.

자바 16부터는 이렇게 고칠 수 있어요.

Suggested change
.collect(Collectors.toList());
.toList();

Copy link
Author

Choose a reason for hiding this comment

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

자바 11 이후의 문법을 아직까지 몰랐는데, 미션을 진행해보면서 틈틈히 공부해봐야겠네요!
꼼꼼한 피드백 감사드려요!

Copy link
Author

Choose a reason for hiding this comment

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

2단계 미션에 반영해두겠습니다!

@mj950425 mj950425 merged commit a299579 into next-step:devfancy Jan 27, 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