Skip to content

[박종범] 1차시 미션 제출입니다. #104

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

Open
wants to merge 16 commits into
base: asa9874
Choose a base branch
from

Conversation

asa9874
Copy link

@asa9874 asa9874 commented Jul 31, 2025

1. 계산기

별도의 설명이 없는 +-*/연산이 가능한 간단한 계산기입니다.

2. String계산기

숫자들의 합을 구현할때 stream을 사용하였습니다.
하지만 막상 stream으로 구현해보니 내부에 if문을 통해 예외를 던지는 부분이 많아서 for문으로 작성하는게 더 깔끔해 보이지않았나 생각합니다. 이부분에 대해 의견 부탁드립니다.

3,4. 테스트

테스트 클래스는 외부로 공개되는 기능을 기준으로 나눴습니다. 간단한계산기(+,-,*,/), stirng계산기(+)

테스트마다 새로운 인스턴스를 생성할요소는 없어서 @Before 은 사용하지않았고, 결과값 비교, 예외확인 테스트만 진행하였습니다.

AssertJ 테스트의 경우 Given-When-Then 패턴을 참고하여 작성하였습니다.

Given의 경우 불변이라 생각하여 CalculatorAssertJTest에서는 final을 붙였지만 작성후 오히려 테스트 코드의 가독성을 떨어트리지않았나 생각이 들게 되어, 비교용으로 StringCalculatorAssertJTest에서는 final을 사용하지않고 작성해봤는데 비교후 어느 패턴이 더 괜찮아 보이는지 의견 말씀해주시면 감사합니다.

Copy link

@BaeJinho4028 BaeJinho4028 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 25 to 27
if (!isNumber(num)) {
throw new RuntimeException("숫자가 아닌 값이 포함되어 있습니다");
}

Choose a reason for hiding this comment

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

if문에서 예외를 던지는 것도 메소드로 분리를 해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

제가 주로 검증 관련된 로직을 분리할때는 다른 클래스에도 재활용하고 여러 조건과 조합하는등 확장성을 고려해서 boolean 반환으로 하고 예외반환은 비즈니스 로직을 수행하는 쪽에서 작성하는 방향으로 작성하고있는데 해당부분에 대해 의견이 궁금합니다!

Choose a reason for hiding this comment

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

그럼 기존 bool 메서드를 재활용해서 void로 예외만 반환하는 메서드를 추가로 만드는건 어떨까요? SRP 원칙대로 검증은 해당 클래스에서만 하면 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

넵 해당 방식으로 리팩토링 해보겠습니다.

Comment on lines 29 to 31
if (number < 0) {
throw new RuntimeException("음수는 허용되지 않습니다");
}

Choose a reason for hiding this comment

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

(위와 동일)

Comment on lines 10 to 12
if (isEmptyOrNull(input)) {
return 0;
}

Choose a reason for hiding this comment

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

StringUtils의 메서드도 한번 써보면 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

apache.commons.lang3 의 StringUtils는 이번에 처음알게 되었네요.
해당 의존성추가후 리팩토링 해보겠습니다!

Choose a reason for hiding this comment

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

아마 Junit것도 있는걸로 알고있어요.
프로젝트에서는 springframework 거를 쓰는편입니다

import java.util.Arrays;

public class StringCalculator {
private final String delimiter = ",|:";

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.

해당 부분에 대해서는 놓치고 있었네요.
static 으로 클래스 상수로 선언하여 메모리를 아낄수 있게 리팩토링 하겠습니다.

Comment on lines 23 to 34
private int getSum(String[] numbers) {
return Arrays.stream(numbers).mapToInt(num -> {
if (!isNumber(num)) {
throw new RuntimeException("숫자가 아닌 값이 포함되어 있습니다");
}
int number = Integer.parseInt(num);
if (number < 0) {
throw new RuntimeException("음수는 허용되지 않습니다");
}
return number;
}).sum();
}

Choose a reason for hiding this comment

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

Calculator 에서 덧셈 기능을 만들었다면 이를 재활용해보는 방법도 있지 않을까요??

Copy link
Author

Choose a reason for hiding this comment

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

해당 부분에 대해서 코드 작성할때 고려해보았는데, 기본 제공하는 메서드인 sum으로 구현하는 대신 누적합 형태로 Calculator의 덧셈 기능을 사용하는것은 불필요한 코드가 늘어난다고 생각되어 해당 방식으로 작성하였습니다.

Comment on lines 14 to 18
if (hasCustomDelimiter(input)) {
int endCustomDelimiterIndex = input.indexOf('\n');
delimiter += "|" + input.substring(2, endCustomDelimiterIndex);
input = input.substring(endCustomDelimiterIndex + 1);
}

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.

해당 부분도 extractCustomDelimiter로 로직을 분리하는 방향으로 리팩토링 진행하겠습니다.

Comment on lines 18 to 19
final int a = 2;
final int b = 3;

Choose a reason for hiding this comment

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

2번 질문 답변

final로 하면 데이터 정합성을 유지할 수 있지만 가독성을 해칠 수도 있겠죠. 정답은 없고 결국 트레이드 오프라고 생각합니다.
저는 굳이 final을 붙이지는 않는 편이예요

밑에 이전 선배님도 비슷한 고민을 한 글이 있길래 남겨봅니다.

https://prod.velog.io/@junho5336/%ED%95%A8%EC%88%98-Parameter%EC%9D%98-final-%ED%82%A4%EC%9B%8C%EB%93%9C

Copy link
Author

Choose a reason for hiding this comment

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

저도 가독성 부분에서 해당 부분이 조금 신경쓰이는 부분이 더 큰거같아서
final을 제거하는 방향으로 리팩토링 진행해보겠습니다.

Comment on lines 24 to 31
return Arrays.stream(numbers).mapToInt(num -> {
if (!isNumber(num)) {
throw new RuntimeException("숫자가 아닌 값이 포함되어 있습니다");
}
int number = Integer.parseInt(num);
if (number < 0) {
throw new RuntimeException("음수는 허용되지 않습니다");
}

Choose a reason for hiding this comment

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

1번 질문 답변

확실히 stream을 쓰는것보다 for문이 더 가독성이 좋아보일떄도 실제로 많죠.
하지만 해당 stream 내부 로직 전체를 메서드 분리로 모듈화해서 가독성을 높일 수는 없을지도 고민해보면 좋을 것 같아요.
게다가 stream의 중괄호 스코프와 if문의 중괄호 스코프가 같이 들어가 있으니 depth가 늘어나는 문제도 있구요.

다양한 방법을 고민해보셨으면 좋겠습니다.👍

Comment on lines +7 to +16
public class CalculatorAssertJTest {

private final Calculator calculator = new Calculator();

@Nested
@DisplayName("덧셈 테스트")
public class AddTest {

@Test
public void 정수_더하기() {

Choose a reason for hiding this comment

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

해당 부분은 클래스 시작할때 들여쓰기가 있는데, 다른 부분에는 없네요.
추후 협업을 하는데 코드 컨벤션은 매우 중요한 요소이기에 기준을 잘 정하셨으면 좋겠습니다
(참고로 코인은 클래스 시작은 들여쓰기O, 메서드 시작은 들여쓰기X입니다)

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 해당 부분은 코인 방식으로 리팩토링 해보겠습니다.

인텔리제이에 해당 bcsd 컨벤션 포매팅 파일 을 적용해서 사용하였는데, 해당 포매팅파일 말고도 별도로 더 강하게 컨벤션을 잡기 위해 사용하고 계신 툴이 혹시 있으신지 궁금합니다.

int number = Integer.parseInt(num);
validateIsPositive(number);
return number;
}).sum();
Copy link

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.

3 participants