[4단계 - 리팩터링] 권장순 과제 제출합니다. #91

jsoonworld · 2025-04-30T06:54:40Z

💬 4단계 - AssertJ 기반 테스트 리팩터링 중 고민된 부분을 공유드립니다!

안녕하세요 오찌! 😄

이번 단계에서는 기존 JUnit5 기반의 테스트 코드를 모두 AssertJ 기반으로 리팩터링하는 작업을 진행했습니다.
기능상의 변화는 없지만, 테스트 표현의 가독성과 의도 표현 방식에 집중해 보았습니다.

그 과정에서 아래와 같은 고민이 생겨, 오찌의 의견을 듣고 싶습니다! 🙏

1. `assertThat(expected).isEqualTo(actual)` vs `assertThat(actual).isEqualTo(expected)`?

이번 리팩터링 과정 중 헷갈렸던 포인트 중 하나입니다.
기존 assertEquals(expected, actual) 구조에 익숙하다 보니, 그대로 변환하면 assertThat(expected).isEqualTo(actual)이 되는데
AssertJ는 일반적으로 actual → expected 순을 따르는 듯했습니다.

assertThat(6).isEqualTo(calculator.calculateSum("1,2:3")); // 권장 방식

📌 오찌는 이 순서를 실제 현업에서 어떻게 사용하고 계신지 궁금합니다!
테스트 가독성 차원에서 팀 차원의 컨벤션이 있다면 더 배워보고 싶습니다.

2. `isZero()` 같은 단축 표현을 언제 활용할지

AssertJ는 isZero(), isTrue(), isEmpty() 같은 fluent한 표현이 많아 코드가 더 자연스럽게 읽히는 장점이 있었습니다.
다만 일부는 isEqualTo(0)보다 오히려 표현 의도가 모호해지는 느낌도 있었는데요:

assertThat(calculator.calculateSum("")).isZero(); // vs isEqualTo(0)

📌 이런 메서드들은 어떤 기준으로 사용하면 좋을까요?
단순 값 비교일 땐 isEqualTo가 더 직관적일까요?

3. 예외 테스트: `assertThatThrownBy` 사용 시 한 줄로 끝내는 것이 좋은가?

다음과 같이 한 줄로 처리 가능한 부분이 있었는데요:

assertThatThrownBy(() -> calculator.calculateSum("1,-2,3"))
    .isInstanceOf(IllegalArgumentException.class);

기존 JUnit 방식(assertThrows)보다 훨씬 직관적이고 확장 가능한 느낌이었지만,
오히려 Exception 메시지까지 검증할 경우에는 assertThrows가 더 명확하다고도 느꼈습니다.

📌 예외 메시지나 내용까지 검증이 필요한 경우, 어떤 방식을 선호하시나요?

4. `contains` vs `isEqualTo` : 문자열 비교에서의 의도 표현

OutputView 테스트에서 문자열을 비교할 때 contains()를 주로 사용했습니다.
예: "계산 결과: 10" 출력 여부

assertThat(output).contains(OutputView.RESULT_MESSAGE + result);

하지만 이 방식은 전체 문자열이 정확히 같은지는 보장하지 못한다는 점이 아쉬웠고,
그렇다고 전체 문자열을 isEqualTo()로 검증하면 입력이나 출력 포맷이 바뀔 때 너무 취약해진다는 고민도 들었습니다.

📌 출력 테스트 시에는 보통 어떤 기준으로 비교 전략을 선택하시나요?

5. AssertJ 전환의 기준: 모든 테스트를 바꾸는 것이 맞는가?

단위 테스트 중 일부는 assertJ로 바꾸는 게 오히려 간결하지 않거나 의미 전달이 떨어지는 느낌도 있었습니다.
예를 들어 assertEquals(new Number(42), number)는
assertThat(number).isEqualTo(new Number(42))보다 명확한가? 하는 고민이 있었습니다.

📌 전환의 우선순위나 기준이 있다면 알려주시면 감사하겠습니다!

Ohzzi

수고하셨습니다! 😄 굉장히 깔끔한 코드를 빠른 시간 내에 완성해주셨군요. 코드의 몇몇 부분에서는 장순님께서 하신 학습과 쌓으신 지식을 엿볼 수 있었습니다. (숫자를 감싼 클래스라든가, 컬렉션을 감싼 클래스의 사용이라든가, instanceof 사용 같은 것들이요.) 다만, 어떠한 패턴이나 기술도 사용 의도가 명확해야 올바른 사용을 하는거라고 생각하기 때문에 몇가지 리뷰를 남긴게 있으니 참고 부탁드려요.

추가로 3단계 PR에 남겨주신 질문과 4단계 PR에 남겨주신 질문에 답변드릴게요!!

StringCalculator의 책임 범위가 너무 넓은가?

저는 넓다고 생각하지 않습니다! 작성해주신 StringCalculator의 로직을 보면 해당 클래스가 자체적으로 가지는 로직은 크게 없고, 대부분 다른 클래스의 메서드를 호출하고 반환값을 받아서 해당 값으로 다시 다른 메서드를 호출하는 역할을 하고 있는 것을 볼 수 있습니다. 이정도는 단일책임원칙 위반은 아니라고 보이고 오히려 parse나 split 같은 로직을 다른 클래스로 적절히 잘 위임하고 StringCalculator는 단순 호출만 하기 때문에 좋아보여요!

CustomDelimiterParser의 위치와 역할

저는 충분히 제 역할을 하고 있는 Parser라고 생각합니다. Delimiters 같은 경우에는 구분자를 모아놓은, 즉 도메인을 모아놓은 컬렉션이기 때문에 파싱의 역할을 하는 것은 적절치 못하다고 생각하고요. 결국 String -> Delimeters, Numbers로 적절히 파싱해주는 역할이 필요한데 이 역할을 CustomDelimiterParser가 잘 해주고 있는 것 같아요. StringCalculator의 내부 유틸일 필요는 없을 것 같고, 외부 유틸이면 충분할 것 같습니다.

다만 아쉬운 부분은 리뷰에 남긴 것처럼 validation이 일부 허술한 부분이 있는 것을 꼽을 수 있을 것 같고, optional한 제안으로는 어차피 구분자 파싱 -> 다시 구분자 들어있는 문자열로 숫자표현 파싱 의 과정을 거칠거라면 메서드 하나에 몰아넣을 수 있지 않을까 하는 고민을 해보시는 것도 좋을 것 같아요!

Delimiters에서 구분자를 추가하는 방식

성능상 문제 -> 이정도로는 큰 문제는 없습니다. 특히나 커스텀 구분자의 존재 때문에 구분자가 계속 달라질 수 있다는 점에서 split은 괜찮은 선택으로 보여요.
Delimiters가 리스트를 넘기는 것이 패턴을 넘기는 것 보다 더 자연스럽습니다. split의 주체가 Delimiters가 아닌 이상, 어떤 형태로 split 하는지는 Delimiters가 알 필요가 없고, 때문에 정규식과 관련된 것은 모르는게 논리적으로 좀 더 타당하다고 볼 수 있어요.

객체지향에 대한 이야기를 해주셨는데, 모든 로직을 다 객체지향적으로 만드는 것이 효율적이지는 않다고 생각해요. 어떤 부분에서는 절차지향적 코드가 더 효율적일 수 있고, 객체지향을 하더라도 유틸리티를 매 번 객체로 만들 필요는 없겠죠. 어떨 때 객체로 만들고 어떨 때 유틸리티를 만들어 사용할 것인지 장순님만의 기준을 세울 수 있도록 코드를 이렇게도 저렇게도 작성해보고 스스로 많은 고민을 해보는게 도움이 될 것 같아요.

Number가 유효성 검증 책임을 갖는 것이 좋은가?

이전 PR에서 말씀드려서 긴 설명은 하지 않을게요. Number라는 도메인이 가지는 규칙에 대해서는 도메인에서 검증한다를 원칙으로 가져가는게 좋을 것 같습니다! 이 부분에 대해서 추가적인 의견이 필요하면 코멘트 남겨주세요!

입력값 분리 흐름이 적절한가?

저는 그렇게 나쁜 흐름은 아니라고 생각해요. 요구사항이 늘어날수록 분기(if)가 늘어나는 것은 당연하기 때문에 지금 상황에서는 별로 신경쓰지 않아도 될 것 같습니다. 좀 더 깔끔하고 논리적 허점이 없게 표현하고 싶다면 위에 제가 남긴 optional한 코멘트를 고민해보셔도 도움이 될거에요!

assertThat(expected).isEqualTo(actual) vs assertThat(actual).isEqualTo(expected)?

무조건 후자입니다. 애초에 assertThat의 파라미터 이름이 actual 이기도 하고요, 이게 isEqualTo라는 동등비교일 때는 순서가 바뀌어도 문제가 안되지만, isTrue, isFalse, contains와 같은 다른 체이닝 메서드를 사용할 때를 생각해보면 답이 나올거에요.

저는 실제 현업에서 코틀린을 사용하기 때문에 assertJ를 사용하지는 않는데요, 코틀린에서의 assertion 역시 actual이 앞에 오는 형태를 띄고 있습니다.

테스트 컨벤션 같은 경우는 사실 정하지 않는 팀도 많고(테스트를 심지어는 안짜는 사람도 많으니까요), 취향이 굉장히 많이 갈리는 부분인데요. 정답은 아니지만 그나마 널리 퍼져있는 컨벤션? 같은 느낌은 given, when, then을 적절히 잘 나눠주는 형태의 테스트인 것 같습니다. given-when-then 패턴에 대해서 자료를 찾아보시는 것을 추천드려요.

isZero() 같은 단축 표현을 언제 활용할지

저는 isZero, isPositive, isNegative 등의 메서드를 적극적으로 사용합니다. 테스트든 프로덕션 코드든 상관 없이요. 그런데 이건 개인 취향의 영역이고, 뭐가 더 직관적인지도 사람들마다 다른 것 같아요. 장순님 편한대로 사용하셔도 무방하다고 생각합니다.

예외 테스트: assertThatThrownBy 사용 시 한 줄로 끝내는 것이 좋은가?

        assertThatThrownBy(() -> NumberParser.parse("abc"))
                .isInstanceOf(IllegalArgumentException.class)
                .message()
                .isEqualTo("[ERROR] 입력은 숫자여야 합니다.");

isInstanceOf는 자기 자신을 그대로 반환하기 때문에 이렇게 쓰시면 됩니다. 이런 식으로 assertj의 메서드들로 다 해결이 되기 때문에 jupiter의 assertion은 안쓴다고 보시면 될 것 같아요. 근데 장순님이 jupiter의 assertion이 더 직관적이고 사용이 편하다고 느끼는 경우가 있다면, 해당 경우에 대해서는 쓰셔도 전혀 문제가 없습니다. 애초에 틀린 assertion이 아니니까요.

contains vs isEqualTo : 문자열 비교에서의 의도 표현

저라면 전체 문자열을 isEqualTo로 검증하겠습니다. 입력이나 출력 포맷이 바뀔 때 너무 취약해진다는 고민을 해주셨는데요, 입력이나 출력 포맷이 바뀌는 것 또한 테스트에서 검증해야 할 부분이라고 생각해요. 상수들까지도 테스트에서는 문자열 리터럴이나 숫자로 사용해서 변경을 감지하는 경우도 있고, 저 역시 해당 방법을 선호합니다. 물론... 변경에 취약한 것은 맞아서 매우 귀찮긴 합니다. 그런데 굳이 프로덕션 코드도 아닌데 변경에 강해야 할 필요는 없고, 의도치 않은 변경을 잡아내는데 더 효과가 있어서 테스트에서는 좀 더 명확한 검증을 할 수 있는 방법을 선택하는게 좋다고 생각합니다.

AssertJ 전환의 기준: 모든 테스트를 바꾸는 것이 맞는가?

바로 위에도 말씀드렸지만 기존 assertion이 더 명확하다고 생각하면 바꾸지 않아도 됩니다! 그런데 assertEquals(new Number(42), number)랑 assertThat(number).isEqualTo(new Number(42))는 제 눈에는 후자가 더 명확해보이긴 하는군요 ㅎ.ㅎ

Ohzzi · 2025-04-30T15:11:43Z

src/main/java/domain/Calculator.java

+        }
+        return firstNumber / secondNumber;
+    }
+}


EOF 문제가 발생했네요. 코드의 마지막에는 반드시 개행이 한 번 있어야 합니다! (즉, 여기서는 26번의 공백 라인이 있어야 하는 것이죠)

Ensure every saved file ends with a line break

옵션에 대해 찾아보시면 도움이 될 것 같아요!

오호, EOF 개념에 대해 다시 한번 정확히 짚고 가게 됐네요!
말씀해주신 옵션도 확인해보고, 앞으로는 저장 시 자동 개행이 적용되도록 설정해두겠습니다!
알려주셔서 감사합니다 🙏

Ohzzi · 2025-04-30T15:18:52Z

src/main/java/domain/Number.java

+
+import java.util.Objects;
+
+public class Number {


클래스 이름이 Number인데요, 일반적으로 우리가 생각하기로는 Number는 모든 숫자를 포함할 것 같은데, 클래스 명만 보고는 음수를 포함하지 않는다는 요구사항을 생각해내기 어렵지 않을까요? 조금 더 구체적인 네이밍이 된다면 좋을 것 같아요.

맞아요! 사실 처음엔 positiveNumber처럼 구체적인 이름도 고민했었는데,
그때는 너무 디테일한 네이밍인가 싶어서 결국 Number로 정했거든요 😅

그런데 말씀 듣고 보니, 요구사항을 고려했을 때는 제너럴한 이름보다 오히려 명확하게 드러나는 이름이 더 적절하겠다는 생각이 드네요.
이 부분도 다시 네이밍 개선해보겠습니다! 피드백 감사합니다 🙏

Ohzzi · 2025-04-30T15:19:36Z

src/main/java/domain/Number.java

+    public int value() {
+        return value;
+    }


일반적으로 getter의 이름은 getXXX의 형태를 많이 사용하는데요, 필드명을 그대로 메서드명으로 사용하신 이유가 궁금합니다!

사실 이 부분은 저도 아직 명확하게 기준이 잡히진 않았어요! 😅
VO를 사용하는 건 굉장히 좋은 방향이라고 생각하지만, 그 안에서 get이라는 네이밍이 들어가면
어떤 순간엔 단순 필드 공개처럼 느껴져서 getter annotation을 쓴 건가? 혹은 깊은 고민 없이 작성한 건가? 라는 생각이 들더라구요.

그래서 VO 내부의 값을 꺼낼 때는 되도록 value()처럼 의도를 좀 더 드러내는 네이밍을 선호하게 됐어요.
예를 들어 Car가 Name을 갖고 있을 때 car.name().value()가 car.getName().getName()보다
조금 더 도메인스럽고 자연스럽게 읽힌다고 느껴졌습니다.

그래서 지금도 value() 같은 표현을 일관되게 사용해보려고 하는 중인데,
혹시 이런 방식에 대해서는 어떻게 생각하시나요?

사실 네이밍에 정답은 없습니다. 예를 들어 자바의 Map에서 모든 value들을 꺼내는 메서드 이름은 getValues()가 아니라 values()이죠.

다만 일반적으로는 getter는 getXXX()의 형태로 작성하는 사람들이 많고, 그게 일종에 국룰처럼 되어 온 것은 맞습니다. 왜냐면 자바 빈 규약에 get+필드명(첫 글자 대문자)로 getter를 만들도록 되어 있기 때문이죠. 해당 규약이 강제성이 있는 것은 아니기 때문에 강제는 아니지만 일반적으로는 이렇게 작성한다~ 정도 알아놓고 넘어가면 좋을 것 같아요.

개인적인 의견을 물으신다면 저라면 그냥 규약에 맞춰서 작성할 것 같습니다! 나중에 가면 Lombok이라는 편의성을 증대시켜주는 플러그인도 사용하게 될텐데, 해당 플러그인도 (커스텀이 가능하지만) get+필드명의 형태로 getter를 만들어주거든요.

절대적인 기준이 없는 상황에서 자바 빈 규약, 도구(Lombok) 사용, 팀 내 협업 등을 종합적으로 고려하면 getXXX 형태의 컨벤션을 따르는 것이 더 적절하겠다는 생각이 드네요! 말씀 주신 내용 참고해서 적용해보겠습니다 😊

Ohzzi · 2025-04-30T15:20:28Z

src/main/java/domain/Number.java

+    @Override
+    public boolean equals(Object o) {
+        if (this == o) return true;
+        if (!(o instanceof Number)) return false;


제가 알기로 IDE에서 equals 오버라이딩을 했을 때 instanceof가 사용되지는 않는 것으로 알고 있는데, 따로 instanceof로 수정해주신건가요? 맞다면, 이유가 궁금합니다!

네! IDE에서 자동 생성하면 보통 getClass()를 사용해서 클래스 일치를 엄격하게 비교하던데,
이번에는 의도적으로 instanceof를 사용해서 조금 더 유연하게 비교할 수 있도록 수정해봤어요.

물론 현재 구조에서는 Number가 상속될 일은 거의 없지만,
VO나 도메인 객체를 다룰 때는 동등성 비교가 "같은 타입이냐"보다는 "같은 의미를 가지느냐"에 더 가깝다고 생각해서
이번엔 비교 기준을 조금 느슨하게 가져가는 쪽을 선택했습니다.

다만 이렇게 instanceof로 처리하는 게 지금 상황에서는 과한 설계일 수도 있지 않나? 하는 생각도 들어요 😅
혹시 이 부분에 대해 다른 관점이나 추천하시는 방식이 있다면 의견 듣고 싶습니다! ㅎㅎ

나중에 스프링을 하고 JPA를 사용하게 되면 이 getClass와 instanceof로 인한 문제점을 만나게 될 수 있을거에요. 현재 수준에서는 getClass와 instanceof로 인한 차이가 발생하지 않을테니, 편하신대로 작성해주셔도 좋습니다! getClass를 사용했을 때와 instanceof를 사용했을 때 어떻게 달라지는지 정도만 인지하고 넘어간다면 충분할 것 같아요!

Ohzzi · 2025-04-30T15:26:23Z

src/main/java/domain/Numbers.java

+import java.util.List;
+import java.util.stream.Collectors;
+
+public class Numbers {


Number들을 모은 Numbers 라는 클래스를 만드셨군요. 이 클래스를 만들게 된 이유와 필요하다고 생각하는 이유를 설명해주실 수 있을까요?

Numbers를 만든 이유는 구분자를 제거한 숫자 값들을 한 곳에 모아 관리하고 싶었기 때문이에요.
단순히 리스트로만 다루기보다는, 나중에 이 값들을 조합하거나 가공하는 일이 생겼을 때
명확한 책임을 가진 일급 컬렉션으로 묶어두는 게 더 유연하게 확장할 수 있겠다는 판단이 들었습니다.

예를 들어 이번 요구사항에서도 숫자들의 합계를 구하는 로직이 있었는데,
이처럼 값들이 한 곳에 잘 모여 있다면 계산이나 필터링 같은 처리도 더 명확하게 표현할 수 있을 거라고 생각했어요.
List보다 Numbers라는 객체로 묶여 있으면 도메인적인 의미도 더 잘 전달될 수 있을 것 같고요!

혹시 이 구조에 대해 개선할 만한 방향이 보이신다면 의견 주시면 감사하겠습니다! 🙏

적절한 의도를 가지고 일급 컬렉션을 만들어주셨군요. 지금 요구사항을 기준으로는 이 일급 컬렉션이 적절한 역할을 수행하고 있기 때문에 지금 구조에 대한 이야기를 할 필요는 없을 것 같습니다! 저는 좋은 선택이었다고 생각해요!

Ohzzi · 2025-04-30T15:40:39Z

src/main/java/domain/CustomDelimiterRegister.java

@@ -0,0 +1,14 @@
+package domain;
+
+public class CustomDelimiterRegister {


이미 Delimiters라는 일급 컬렉션 클래스가 있기 때문에 CustomDelimiterRegister는 지금 delimiters를 감싸고 있을 뿐 그 자체의 역할이 없다고 생각해요. 그런 의미에서 이 클래스가 불필요한 클래스는 아닌가 고민해보시면 좋을 것 같아요!

커스텀 구분자와 기본 구분자를 구분해서 관리하고 싶다는 의도로 만들었는데,
말씀 듣고 보니 정말 말씀하신 대로 Delimiters를 단순히 감싸고만 있는 구조였던 것 같아요! 😅
지금 형태라면 굳이 분리할 필요 없이 Delimiters 내부에서 처리하는 게 더 적절하겠다는 생각이 듭니다.
피드백 감사합니다! 이 부분은 구조를 다시 한번 개선해보겠습니다 🙏

Ohzzi · 2025-04-30T15:45:41Z

src/main/java/utils/CustomDelimiterParser.java

+    }
+
+    public String parseCustomDelimiter(String expression) {
+        validate(expression);


validation 굿입니다!

감사합니다~! 😄

Ohzzi · 2025-04-30T15:47:29Z

src/main/java/utils/CustomDelimiterParser.java

@@ -0,0 +1,30 @@
+package utils;
+
+public class CustomDelimiterParser {


이 클래스는 멤버 변수를 갖지 않고, 메서드들은 파라미터로 들어온 값만 사용합니다. 때문에 이 클래스가 가지는 메서드들이 static method여도 무방할 것 같은데 어떻게 생각하시나요?

네! 말씀해주신 대로 파라미터로만 동작하고 상태를 가지지 않는 클래스이기 때문에,
static 메서드로 바꾸는 것도 좋은 선택이라고 생각해요!

다만 저도 한 가지 고민됐던 점이 있는데요 —
static은 클래스가 로딩될 때 메모리에 올라간다고 알고 있어서,
사용하지 않을 수도 있는 값들까지도 미리 힙에 올라가면 낭비가 되지 않을까? 하는 고민이 들더라고요 😅
실제로 성능에 큰 영향을 미칠 정도는 아닐 수도 있겠지만,
이런 부분도 고려해서 꼭 필요한 유틸 클래스에만 static을 적용하려고 조심하는 편이에요.

혹시 이런 고민에 대해서는 어떻게 생각하시나요?
static 사용에 있어서 기준이나 경험 있으시다면 공유해주시면 감사하겠습니다! 🙏

static으로 한 번 메모리에 올라가기 vs 사용할 때마다 힙에 새 객체를 만들기를 비교해본다면, 후자가 더 메모리에 부담이 되는거라고 볼 수 있어요. 사용하지 않을 수도 있는 값들까지도 미리 힙에 올라가면 낭비가 되지 않을까? 하는 고민이 들더라고요 😅 라고 남겨주셨는데, static으로 선언한 메서드나 필드는 힙에 올라가지 않습니다! 어디에 올라가는지는 개인적인 학습을 진행해보시면 좋을 것 같아요. 그리고 대부분의 메모리 관련된 문제는 힙에서 발생하기 때문에 메모리 관점에서는 힙에 계속 새 객체를 만드는 행동이 더 낭비라고 볼 수 있을 것 같습니다.

저는 메서드가 인스턴스에 종속되는 경우가 아니라면 static으로 만드는 편이에요. 인스턴스의 값을 사용하거나, 호출을 클래스 레벨이 아닌 인스턴스 레벨에서 진행하는 것이 맥락상 자연스러운 경우(사실 이런 경우가 대부분은 인스턴스의 멤버 변수를 사용하는 경우이긴 합니다.) 꼭 필요한 유틸 클래스에만 static을 적용하려고 한다 하셨는데, 이 클래스의 경우 사실은 parsing을 하는 유틸성 클래스이기에 더더욱 static이 맞다고 생각해요. 프로그램 내에 여러 개의 CustomDelimiterParser의 인스턴스가 존재할 때, 그 모든 인스턴스들은 정확히 일치하는 동작을 하잖아요? 그러면 인스턴스화 될 필요가 없는 클래스라고 볼 수 있겠죠.

사용할 때마다 힙에 새 객체를 만드는 게 오히려 메모리에 더 부담이 된다는 건 새롭게 알게 된 사실이네요!
그리고 static으로 선언된 메서드나 필드는 힙에 올라가지 않는다는 부분도 알려주셔서, 이 부분은 다시 한번 공부해보겠습니다 😊

또 "프로그램 내에 여러 개의 CustomDelimiterParser 인스턴스가 존재할 때, 그 모든 인스턴스들은 정확히 일치하는 동작을 하잖아요?" 라는 설명이 정말 인상 깊었어요. 말씀해주신 비유 덕분에 훨씬 더 와닿았던 것 같아요!

앞으로 말씀 주신 내용 참고해서 좀 더 깔끔하게 다듬어보겠습니다 🙏

Ohzzi · 2025-04-30T15:57:59Z

src/main/java/utils/CustomDelimiterParser.java

+    public String parseNumbersExpression(String expression) {
+        int delimiterEndIndex = expression.indexOf(CUSTOM_DELIMITER_SUFFIX);
+        return expression.substring(delimiterEndIndex + 1);
+    }


코드상으로는 커스텀 구분자가 없는 경우도 대응할 수 있겠네요. 하지만 이 객체가 CustomDelimiterParser이기 때문에, 커스텀 구분자가 없는 expression이 들어왔을 때 정상 요청으로 볼 것인가에 대해 생각해봤으면 합니다! 반드시 validation을 해야 한다고 말씀드리는 것이 아니에요. 정상요청과 비정상요청에 대해 장순님이 명확한 기준을 세우기를 바라며 드리는 리뷰입니다!

아하, 맞아요! 사실 이번 미션을 구현하면서 그런 부분들이 제일 애매하게 느껴졌던 지점이었습니다 😅
요구사항이 "이럴 땐 이렇다"는 식으로 명확하게 정의되지 않다 보니,
저 스스로 기준을 세워야 하는데 그 기준이 구현에 잘 녹아들지 못하고 일관성이 부족했던 것 같아요.

말씀해주신 것처럼 다음부터는 이런 흐름에서도 정상/비정상을 나누는 명확한 기준을 먼저 고민하고,
그 기준을 바탕으로 코드 설계까지 자연스럽게 이어지도록 연습해봐야겠다는 생각이 들었습니다!
좋은 방향 제시 감사합니다 🙏

Ohzzi · 2025-04-30T16:04:25Z

src/main/java/utils/ExpressionSplitter.java

+
+    public List<String> split(String expression) {
+        List<String> delimiterList = delimiters.getDelimiters();
+        String regex = String.join("|", delimiterList);


정규표현식을 위해 |로 join을 걸어주셨군요! 그런데 만약 커스텀 구분자가 | 라면 어떻게 될까요?

와..! 정말 생각 못 했던 부분이네요 😅
말씀해주신 것처럼 커스텀 구분자로 | 같은 특수 문자가 들어올 수 있다는 점을 고려하면,
지금처럼 단순 join("|")으로 처리하는 방식은 오히려 예기치 않은 동작을 만들 수 있을 것 같다는 생각이 들었어요.

그래서 오히려 정규식을 직접 구성하지 않고, 해당 부분을 제거하는 방향으로 리팩터링해보는 게 더 안전하고 깔끔할 것 같아요!

넵 좋습니다! 정규식을 사용하는 경우에 대해 살짝 아이디어를 드리면, 정규식에서 |, &와 같이 예약어로 사용되는 문자를 문자 그대로 사용하고 싶을 때는 \\를 사용한다는 힌트를 드리겠습니다! String.split이 정말 편리한 메서드이기 때문에 한 번 정규식을 쓰는 방법도 고민해보시면 좋을 것 같네요!

맞아요! \\를 사용하면 정규식 특수문자도 그대로 사용할 수 있었는데, 완전히 깜빡하고 있었네요 😅
추가로 찾아보니 .map(Pattern::quote) 메서드를 활용하면 이와 같은 상황에도 안전하게 적용할 수 있다는 걸 알게 됐어요.
두 방법 모두 적절한 방식이라고 느껴지지만, 이번에는 새로운 방식인 .map(Pattern::quote)를 적용해서 리팩터링해보겠습니다! 😊

Pattern quote 까지 공부해보셨군요. 자동으로 이스케이프 해주니 더 좋겠네요! 👍

jsoonworld · 2025-05-01T01:11:22Z

@Ohzzi

StringCalculator의 책임 범위가 너무 넓은가?

저는 넓다고 생각하지 않습니다! 작성해주신 StringCalculator의 로직을 보면 해당 클래스가 자체적으로 가지는 로직은 크게 없고, 대부분 다른 클래스의 메서드를 호출하고 반환값을 받아서 해당 값으로 다시 다른 메서드를 호출하는 역할을 하고 있는 것을 볼 수 있습니다. 이정도는 단일책임원칙 위반은 아니라고 보이고 오히려 parse나 split 같은 로직을 다른 클래스로 적절히 잘 위임하고 StringCalculator는 단순 호출만 하기 때문에 좋아보여요!

다행이에요! 말씀해주신 것처럼 StringCalculator가 핵심 로직을 직접 처리하기보다는 역할을 잘 위임하고 있다는 피드백을 들으니 안심이 되네요 😄 책임 분리가 괜찮았다는 점이 자신감을 주는 것 같아요. 감사합니다! 🙏

CustomDelimiterParser의 위치와 역할

저는 충분히 제 역할을 하고 있는 Parser라고 생각합니다. Delimiters 같은 경우에는 구분자를 모아놓은, 즉 도메인을 모아놓은 컬렉션이기 때문에 파싱의 역할을 하는 것은 적절치 못하다고 생각하고요. 결국 String -> Delimeters, Numbers로 적절히 파싱해주는 역할이 필요한데 이 역할을 CustomDelimiterParser가 잘 해주고 있는 것 같아요. StringCalculator의 내부 유틸일 필요는 없을 것 같고, 외부 유틸이면 충분할 것 같습니다.
다만 아쉬운 부분은 리뷰에 남긴 것처럼 validation이 일부 허술한 부분이 있는 것을 꼽을 수 있을 것 같고, optional한 제안으로는 어차피 구분자 파싱 -> 다시 구분자 들어있는 문자열로 숫자표현 파싱 의 과정을 거칠거라면 메서드 하나에 몰아넣을 수 있지 않을까 하는 고민을 해보시는 것도 좋을 것 같아요!

피드백 감사합니다! 🙏
말씀해주신 검증 부분은 다시 보니 정말 아쉽게 작성되었네요. 해당 부분은 다시 꼼꼼하게 점검해보고, 다음 미션에서도 유효성 검증을 좀 더 의식적으로 챙겨보는 습관을 들여야겠다는 생각이 들었습니다!

또, 파싱 → 파싱 구조에 대해 말씀해주신 내용도 정말 인상 깊었어요.
지금까지는 한 메서드에 하나의 책임만 가져야 한다는 생각에 ‘무조건 분리’만 해왔던 것 같은데, 관심사가 같은 로직이라면 오히려 결합해서 하나의 흐름으로 관리하는 게 더 명확한 책임 분리가 될 수도 있겠다는 걸 느꼈습니다.

익숙함을 내려놓고 더 나은 구조를 고민할 수 있게 도와주셔서 감사합니다! 😊

Delimiters에서 구분자를 추가하는 방식

성능상 문제 -> 이정도로는 큰 문제는 없습니다. 특히나 커스텀 구분자의 존재 때문에 구분자가 계속 달라질 수 있다는 점에서 split은 괜찮은 선택으로 보여요.
Delimiters가 리스트를 넘기는 것이 패턴을 넘기는 것 보다 더 자연스럽습니다. split의 주체가 Delimiters가 아닌 이상, 어떤 형태로 split 하는지는 Delimiters가 알 필요가 없고, 때문에 정규식과 관련된 것은 모르는게 논리적으로 좀 더 타당하다고 볼 수 있어요.
객체지향에 대한 이야기를 해주셨는데, 모든 로직을 다 객체지향적으로 만드는 것이 효율적이지는 않다고 생각해요. 어떤 부분에서는 절차지향적 코드가 더 효율적일 수 있고, 객체지향을 하더라도 유틸리티를 매 번 객체로 만들 필요는 없겠죠. 어떨 때 객체로 만들고 어떨 때 유틸리티를 만들어 사용할 것인지 장순님만의 기준을 세울 수 있도록 코드를 이렇게도 저렇게도 작성해보고 스스로 많은 고민을 해보는게 도움이 될 것 같아요.

와우, 이 부분 정말 인상 깊었습니다!

“모든 로직을 다 객체지향적으로 만드는 것이 효율적이지는 않다고 생각해요.”

객체지향 설계가 협업과 유지보수 측면에서 강력한 도구라는 생각을 갖고 있었는데,
한편으로는 말씀해주신 것처럼 과도한 분리로 오히려 복잡해지고 비효율적으로 느껴졌던 순간들도 있었던 것 같아요.

이번 피드백을 통해 무조건적인 객체 분리보다는, 어떤 맥락에서 객체화가 유효한지 먼저 고민하고 적용하는 습관을 길러야겠다는 생각이 들었습니다. 코드를 이렇게도 저렇게도 작성해보며 기준을 세우라는 말씀도 큰 힌트가 되었어요. 감사합니다! 🙏

Number가 유효성 검증 책임을 갖는 것이 좋은가?

이전 PR에서 말씀드려서 긴 설명은 하지 않을게요. Number라는 도메인이 가지는 규칙에 대해서는 도메인에서 검증한다를 원칙으로 가져가는게 좋을 것 같습니다! 이 부분에 대해서 추가적인 의견이 필요하면 코멘트 남겨주세요!

이전 PR에서 말씀해주신 내용, 잘 기억하고 유념하겠습니다!
해당 원칙을 기준 삼아 도메인 책임을 더 명확하게 나눠보도록 하겠습니다. 감사합니다! 🙏

입력값 분리 흐름이 적절한가?

저는 그렇게 나쁜 흐름은 아니라고 생각해요. 요구사항이 늘어날수록 분기(if)가 늘어나는 것은 당연하기 때문에 지금 상황에서는 별로 신경쓰지 않아도 될 것 같습니다. 좀 더 깔끔하고 논리적 허점이 없게 표현하고 싶다면 위에 제가 남긴 optional한 코멘트를 고민해보셔도 도움이 될거에요!

좋은 말씀 감사합니다~!
말씀해주신 코멘트도 함께 고민해보면서 흐름을 더 깔끔하게 다듬어보겠습니다! 😊

assertThat(expected).isEqualTo(actual) vs assertThat(actual).isEqualTo(expected)?

무조건 후자입니다. 애초에 assertThat의 파라미터 이름이 actual 이기도 하고요, 이게 isEqualTo라는 동등비교일 때는 순서가 바뀌어도 문제가 안되지만, isTrue, isFalse, contains와 같은 다른 체이닝 메서드를 사용할 때를 생각해보면 답이 나올거에요.
저는 실제 현업에서 코틀린을 사용하기 때문에 assertJ를 사용하지는 않는데요, 코틀린에서의 assertion 역시 actual이 앞에 오는 형태를 띄고 있습니다.
테스트 컨벤션 같은 경우는 사실 정하지 않는 팀도 많고(테스트를 심지어는 안짜는 사람도 많으니까요), 취향이 굉장히 많이 갈리는 부분인데요. 정답은 아니지만 그나마 널리 퍼져있는 컨벤션? 같은 느낌은 given, when, then을 적절히 잘 나눠주는 형태의 테스트인 것 같습니다. given-when-then 패턴에 대해서 자료를 찾아보시는 것을 추천드려요.

오호! 체이닝 메서드를 고려하면 왜 actual이 앞에 와야 하는지 명확해지네요! ㅎㅎ
말씀 듣고 나니 단순히 컨벤션 문제가 아니라, 표현의 일관성과 확장성에서도 중요한 요소라는 걸 알게 됐습니다!

그리고 하나 궁금한 점이 생겼는데요 —
코틀린에서는 assertJ를 사용하지 않는다고 하셨는데, 그러면 보통 어떤 방식이나 라이브러리를 활용해서 테스트를 작성하시는지 궁금합니다!
KotlinTest나 Kotest, 혹은 JUnit만 사용하는 경우도 있는지 여쭤보고 싶어요 😊

isZero() 같은 단축 표현을 언제 활용할지

저는 isZero, isPositive, isNegative 등의 메서드를 적극적으로 사용합니다. 테스트든 프로덕션 코드든 상관 없이요. 그런데 이건 개인 취향의 영역이고, 뭐가 더 직관적인지도 사람들마다 다른 것 같아요. 장순님 편한대로 사용하셔도 무방하다고 생각합니다.

오홍 그렇군요! 😄
말씀해주신 것처럼 이 부분은 직접 사용해보면서 어떤 표현이 더 직관적인지, 가독성이 좋은지 체감해보는 게 중요할 것 같아요.
저도 앞으로는 조금 더 적극적으로 활용해보면서 저만의 취향과 기준을 찾아가보겠습니다! 감사합니다 🙏

예외 테스트: assertThatThrownBy 사용 시 한 줄로 끝내는 것이 좋은가?

        assertThatThrownBy(() -> NumberParser.parse("abc"))
                .isInstanceOf(IllegalArgumentException.class)
                .message()
                .isEqualTo("[ERROR] 입력은 숫자여야 합니다.");

isInstanceOf는 자기 자신을 그대로 반환하기 때문에 이렇게 쓰시면 됩니다. 이런 식으로 assertj의 메서드들로 다 해결이 되기 때문에 jupiter의 assertion은 안쓴다고 보시면 될 것 같아요. 근데 장순님이 jupiter의 assertion이 더 직관적이고 사용이 편하다고 느끼는 경우가 있다면, 해당 경우에 대해서는 쓰셔도 전혀 문제가 없습니다. 애초에 틀린 assertion이 아니니까요.

오홍! 사실 저도 assertJ가 더 편하다고 느끼고 있었는데, 이번 리팩터링 요구사항에서 JUnit Jupiter의 assertion을 assertJ로 전환해야 해서 한 번 더 고민해보게 됐어요!
말씀해주신 대로 assertThatThrownBy 체이닝 방식이 확실히 깔끔하고 표현력도 좋아서 앞으로 더 자주 활용하게 될 것 같습니다 😊
피드백 감사합니다~! 🙏

contains vs isEqualTo : 문자열 비교에서의 의도 표현

저라면 전체 문자열을 isEqualTo로 검증하겠습니다. 입력이나 출력 포맷이 바뀔 때 너무 취약해진다는 고민을 해주셨는데요, 입력이나 출력 포맷이 바뀌는 것 또한 테스트에서 검증해야 할 부분이라고 생각해요. 상수들까지도 테스트에서는 문자열 리터럴이나 숫자로 사용해서 변경을 감지하는 경우도 있고, 저 역시 해당 방법을 선호합니다. 물론... 변경에 취약한 것은 맞아서 매우 귀찮긴 합니다. 그런데 굳이 프로덕션 코드도 아닌데 변경에 강해야 할 필요는 없고, 의도치 않은 변경을 잡아내는데 더 효과가 있어서 테스트에서는 좀 더 명확한 검증을 할 수 있는 방법을 선택하는게 좋다고 생각합니다.

오홍…!
"의도치 않은 변경을 잡아내는 데 더 효과적이다" 라는 말씀 정말 인상 깊었습니다!
저도 테스트 코드 역시 프로덕션 코드처럼 잘 관리되어야 한다고 생각해왔지만,
이번 피드백을 통해 테스트의 진짜 가치는 ‘변화를 감지하는 역할’에 있다는 관점을 새롭게 배우게 된 것 같아요.
그런 면에서 보면 isEqualTo를 사용하는 방식이 훨씬 더 의도와 책임이 분명하다는 생각이 듭니다.

말씀 감사합니다! 앞으로는 그런 시선으로도 테스트를 바라보며 작성해보겠습니다 😊

AssertJ 전환의 기준: 모든 테스트를 바꾸는 것이 맞는가?

바로 위에도 말씀드렸지만 기존 assertion이 더 명확하다고 생각하면 바꾸지 않아도 됩니다! 그런데 assertEquals(new Number(42), number)랑 assertThat(number).isEqualTo(new Number(42))는 제 눈에는 후자가 더 명확해보이긴 하는군요 ㅎ.ㅎ

저도 그렇게 생각되네요! ㅎㅎ
특히 assertThat(number).isEqualTo(...) 쪽이 확실히 눈에 더 잘 들어오는 느낌이에요.
앞으로는 상황에 맞게 더 명확한 표현을 선택해서 써보겠습니다! 😊

jsoonworld

오찌! 꼼꼼하고 따뜻한 피드백 정말 감사합니다 🙏

이번 단계에서는 기능 구현보다도 설계 방향, 책임 분리, 도메인 객체의 의미 표현 같은 부분에서
많이 고민하고, 덕분에 더 깊이 있게 돌아볼 수 있었던 미션이었습니다.

특히 "정상/비정상의 기준을 명확히 하고 코드에 녹여내는 시선",
"VO나 컬렉션의 책임을 어떻게 분리할 것인가"에 대한 피드백들이
저에게 큰 인사이트가 되었습니다!

말씀해주신 내용을 바탕으로 미흡했던 구조와 흐름을 다시 점검하고,
더 일관성 있고 의도가 드러나는 방향으로 리팩터링해보겠습니다!
생각할 거리를 주시는 피드백 덕분에 많이 배우고 성장하는 것 같아요 🙇‍♂️
감사합니다!

jsoonworld · 2025-05-01T01:15:54Z

src/main/java/domain/CustomDelimiterRegister.java

@@ -0,0 +1,14 @@
+package domain;
+
+public class CustomDelimiterRegister {


커스텀 구분자와 기본 구분자를 구분해서 관리하고 싶다는 의도로 만들었는데,
말씀 듣고 보니 정말 말씀하신 대로 Delimiters를 단순히 감싸고만 있는 구조였던 것 같아요! 😅
지금 형태라면 굳이 분리할 필요 없이 Delimiters 내부에서 처리하는 게 더 적절하겠다는 생각이 듭니다.
피드백 감사합니다! 이 부분은 구조를 다시 한번 개선해보겠습니다 🙏

jsoonworld · 2025-05-01T01:17:17Z

src/main/java/domain/Delimiters.java

+        if (customDelimiter == null || customDelimiter.isEmpty()) {
+            return;
+        }


오..! 말씀해주신 부분을 보니 지금 로직이 다소 어색하게 느껴지네요 😅
null이나 빈 문자열은 확실히 delimiter가 될 수 없는 값인데,
현재는 그냥 return만 하고 있어서 정상적인 delimiter와 구분도 안 되고,
실패한 상황인지 아닌지도 판단이 어렵겠다는 생각이 들었습니다.

이 부분은 명확하게 예외를 던지거나, 분기 처리를 분리해서 실패에 대한 피드백을 줄 수 있도록
조금 더 명확한 흐름으로 개선해보겠습니다! 피드백 감사합니다 🙏

jsoonworld · 2025-05-01T01:18:51Z

src/main/java/domain/Delimiters.java

+        if (customDelimiter == null || customDelimiter.isEmpty()) {
+            return;
+        }
+        delimiters.add(customDelimiter);


오..! 정말 그렇네요.
이미 존재하는 구분자를 다시 추가할 경우 중복된 delimiter가 생기게 되는데,
현재 로직에서는 이를 막을 수 있는 제약이나 검증이 전혀 없는 상태라 예상치 못한 동작이 생길 여지가 충분히 있는 것 같아요.

이 부분에 대해서는 고려가 부족했던 것 같습니다.
커스텀 구분자를 받는 로직이다 보니, 중복 여부나 유효성 검사에 좀 더 민감하게 대응했어야 했는데,
단순히 리스트에 추가하는 흐름만 보고 지나친 것 같아요 😅

말씀 주신 피드백을 바탕으로,

이미 등록된 delimiter인지 확인하고

중복 추가를 방지하거나, 중복 시 적절한 예외를 던지도록
검증 로직을 추가해보겠습니다!

감사합니다 🙏

jsoonworld · 2025-05-01T01:24:35Z

src/main/java/domain/Delimiters.java

+    public List<String> getDelimiters() {
+        return new ArrayList<>(delimiters);
+    }


오! 그 부분은 저도 한 번 고민했던 지점인데요 😅
처음엔 외부에서 delimiters 리스트를 직접 수정하는 걸 막고 싶어서 새 리스트를 반환하는 방식으로 방어했는데,
말씀 듣고 보니 이렇게까지 하는 게 과한가? 싶은 생각도 들더라고요.

차라리 List.copyOf()나 Collections.unmodifiableList() 같은 방식이
의도를 더 명확하게 드러낼 수 있었을 것 같다는 생각이 드는데,
혹시 이러한 방식에 대해 어떻게 생각하시나요?

jsoonworld · 2025-05-01T01:31:41Z

src/main/java/domain/Number.java

+
+import java.util.Objects;
+
+public class Number {


맞아요! 사실 처음엔 positiveNumber처럼 구체적인 이름도 고민했었는데,
그때는 너무 디테일한 네이밍인가 싶어서 결국 Number로 정했거든요 😅

그런데 말씀 듣고 보니, 요구사항을 고려했을 때는 제너럴한 이름보다 오히려 명확하게 드러나는 이름이 더 적절하겠다는 생각이 드네요.
이 부분도 다시 네이밍 개선해보겠습니다! 피드백 감사합니다 🙏

jsoonworld · 2025-05-01T01:47:52Z

src/main/java/utils/CustomDelimiterParser.java

@@ -0,0 +1,30 @@
+package utils;
+
+public class CustomDelimiterParser {


네! 말씀해주신 대로 파라미터로만 동작하고 상태를 가지지 않는 클래스이기 때문에,
static 메서드로 바꾸는 것도 좋은 선택이라고 생각해요!

다만 저도 한 가지 고민됐던 점이 있는데요 —
static은 클래스가 로딩될 때 메모리에 올라간다고 알고 있어서,
사용하지 않을 수도 있는 값들까지도 미리 힙에 올라가면 낭비가 되지 않을까? 하는 고민이 들더라고요 😅
실제로 성능에 큰 영향을 미칠 정도는 아닐 수도 있겠지만,
이런 부분도 고려해서 꼭 필요한 유틸 클래스에만 static을 적용하려고 조심하는 편이에요.

혹시 이런 고민에 대해서는 어떻게 생각하시나요?
static 사용에 있어서 기준이나 경험 있으시다면 공유해주시면 감사하겠습니다! 🙏

jsoonworld · 2025-05-01T01:48:28Z

src/main/java/utils/CustomDelimiterParser.java

+    }
+
+    public String parseCustomDelimiter(String expression) {
+        validate(expression);


감사합니다~! 😄

jsoonworld · 2025-05-01T01:50:20Z

src/main/java/utils/CustomDelimiterParser.java

+    public String parseNumbersExpression(String expression) {
+        int delimiterEndIndex = expression.indexOf(CUSTOM_DELIMITER_SUFFIX);
+        return expression.substring(delimiterEndIndex + 1);
+    }


아하, 맞아요! 사실 이번 미션을 구현하면서 그런 부분들이 제일 애매하게 느껴졌던 지점이었습니다 😅
요구사항이 "이럴 땐 이렇다"는 식으로 명확하게 정의되지 않다 보니,
저 스스로 기준을 세워야 하는데 그 기준이 구현에 잘 녹아들지 못하고 일관성이 부족했던 것 같아요.

말씀해주신 것처럼 다음부터는 이런 흐름에서도 정상/비정상을 나누는 명확한 기준을 먼저 고민하고,
그 기준을 바탕으로 코드 설계까지 자연스럽게 이어지도록 연습해봐야겠다는 생각이 들었습니다!
좋은 방향 제시 감사합니다 🙏

jsoonworld · 2025-05-01T01:51:28Z

src/main/java/utils/ExpressionSplitter.java

+
+    public List<String> split(String expression) {
+        List<String> delimiterList = delimiters.getDelimiters();
+        String regex = String.join("|", delimiterList);


와..! 정말 생각 못 했던 부분이네요 😅
말씀해주신 것처럼 커스텀 구분자로 | 같은 특수 문자가 들어올 수 있다는 점을 고려하면,
지금처럼 단순 join("|")으로 처리하는 방식은 오히려 예기치 않은 동작을 만들 수 있을 것 같다는 생각이 들었어요.

그래서 오히려 정규식을 직접 구성하지 않고, 해당 부분을 제거하는 방향으로 리팩터링해보는 게 더 안전하고 깔끔할 것 같아요!

jsoonworld · 2025-05-01T01:51:47Z

src/test/java/domain/CalculatorTest.java

+
+    private final Calculator calculator = new Calculator();
+
+    @ParameterizedTest


감사합니다~! ㅎㅎ

Ohzzi · 2025-05-01T03:44:23Z

그리고 하나 궁금한 점이 생겼는데요 —
코틀린에서는 assertJ를 사용하지 않는다고 하셨는데, 그러면 보통 어떤 방식이나 라이브러리를 활용해서 테스트를 작성하시는지 궁금합니다!
KotlinTest나 Kotest, 혹은 JUnit만 사용하는 경우도 있는지 여쭤보고 싶어요 😊

코틀린 진영에는 Kotest라는 테스트 프레임워크가 있지만, 코틀린 사용자 중에서도 JUnit을 테스트에 활용하시는 분들이 매우 많습니다! 저 역시 자바에서 코틀린으로 넘어갔기도 했고 Kotest의 몇가지 문제 때문에 실무에서는 JUnit을 그대로 사용하고 있는데요. 다만 코틀린에서는 assertJ에서 개선된(!) assertion조차 가독성이 떨어지는 편이어서 저는 assertion만 Kotest의 것을 사용하고 있어요.

살짝 예시를 들어드리면

@Test
fun 테스트() {
    val a = 1
    val b = 2

    a + b shouldBe 3
}

요런 식으로 더 개선된 가독성을 활용할 수 있거든요. 자바에서는 불가능한 방법이니까 그냥 재미로만 봐주시면 좋을 것 같습니다!

Ohzzi

리뷰 반영하느라 고생하셨습니다 장순님~! 다음 미션에서 뵐게요! 연휴 남은 기간동안은 다음 미션에 들어있는 학습 테스트를 먼저 진행해주시면 좋을 것 같아요!

Ohzzi · 2025-05-03T14:00:33Z

src/main/java/domain/PositiveNumber.java

+
+import java.util.Objects;
+
+public class PositiveNumber {


요건 코드 구현사항하고는 다른 문제지만, Positive != NonNegative 입니다! PositiveNumber 라는 클래스라면 0을 포함할 수 없어요!!

NonNegativeNumber 라고 해도 되고, 아예 0 이상이라는 의미를 담기보단 계산기의 계산 대상이라는 느낌의 네이밍으로 가도 괜찮을 것 같아요! 참고로 네이밍에는 정답은 없습니다!

피드백 감사합니다! 🙏

Positive != NonNegative라는 점 명확히 인지하겠습니다!
말씀 주신 것처럼 현재 클래스는 "계산기의 계산 대상"이라는 역할에 더 가깝기 때문에, 그런 의미를 반영한 네이밍이었다면 더 적절했겠다는 생각이 드네요 💡

의견 주셔서 감사합니다 :)

Ohzzi · 2025-05-03T14:01:25Z

src/main/java/domain/PositiveNumber.java

+
+    private static final String ERROR_NEGATIVE_NUMBER = "[ERROR] 숫자는 0 이상이어야 합니다.";
+
+    private final int positiveNumber;


PositiveNumber 안에 value 값으로 positiveNumber 이름이 또 들어가니까 어색한 것 같아요! 보통 한 값을 그대로 감싸는 Value Object의 경우 내부 value 값의 이름은 value 라고 짓는 경우가 많기는 합니다!

피드백 감사합니다! 🙏

사용자 입장에서 봤을 때 PositiveNumber 안에 positiveNumber라는 네이밍이 연속적으로 나오는 게 어색하다는 점 공감됩니다.
다음 미션부터는 이런 부분도 더 신경 써서, VO 내부 값은 value처럼 일반적으로 사용하는 네이밍으로 인지하고 구현해보겠습니다! 💡

의견 감사드려요 :)

jsoonworld added 30 commits April 28, 2025 22:04

docs(README) : 계산기 미션 기능 요구사항 작성

f761dc6

feat(OutputView) : 사용자 입력 및 결과 출력을 위한 출력 기능 구현

86bcbed

feat(InputView) : 사용자 숫자 입력 기능 구현

81fb8ba

feat(Calculator) : 덧셈, 뺄셈, 곱셈, 나눗셈 기능 구현 및 0 나누기 예외 처리

5757e1b

docs(README) : 기능 요구사항 구현 완료 반영

91c60ea

docs(README) : Step2 기능 요구사항 작성

393680e

test(Calculator) : 덧셈 기능에 대한 JUnit5 테스트 코드 작성

25c1699

test(Calculator) : 뺄셈 기능에 대한 JUnit5 테스트 코드 작성

9baf34e

test(Calculator) : 곱셈 기능에 대한 JUnit5 테스트 코드 작성

d003658

test(Calculator) : 나눗셈 기능에 대한 JUnit5 테스트 코드 작성

2cceffb

test(Calculator) : 0으로 나눌 때 예외 발생 테스트 코드 작성

23eb753

refactor(OutputView) : 테스트를 위해 출력 메시지 상수 접근 범위를 default로 변경

c9987be

test(OutputViewTest) : 첫 번째 숫자 입력 프롬프트 출력 테스트 작성

95a2615

test(OutputViewTest) : 두 번째 숫자 입력 프롬프트 출력 테스트 작성

c700c13

test(OutputViewTest) : 계산 결과 출력 테스트 작성

d3e0005

test(InputViewTest) : readNumber 메서드 정상 입력에 대한 테스트 추가

80829b8

refactor(InputView) : 입력과 파싱 책임을 분리하여 입력만 처리하도록 수정

fd6099c

feat(NumberParser) : 문자열 입력을 검증하고 Number 객체로 변환하는 기능 구현

95b22bf

feat(Number) : 숫자 값을 표현하는 VO(Value Object) 클래스 구현

d302d13

test(InputViewTest) : readInput 메서드에 맞게 테스트 메서드 수정

92c7190

test(NumberParserTest) : 문자열 파싱 성공 및 실패 케이스 테스트 작성

18281a9

test(NumberTest) : Number VO에 대한 value, equals 동작 테스트 작성

ad8e38f

docs(README) : 기능 구현 반영

30da163

docs(README) : step3 기능 및 프로그래밍 요구 사항 작성

112be7e

refactor(InputView) : readInput 메서드명을 readExpression으로 변경

7708187

docs(README) : 기능 구현 반영

e9b2dd2

feat(Delimiters) : 기본 구분자 상수화 및 커스텀 구분자 추가 기능 구현

264d73a

docs(README) : 기능 구현 반영

1197396

feat(Delimiters) : 커스텀 구분자 추가 메서드명 수정 및 등록된 구분자 조회 기능 추가

5c51178

test(DelimitersTest) : 기본 구분자 등록 및 커스텀 구분자 추가 테스트 작성

f4b698d

jsoonworld added 23 commits April 29, 2025 16:52

docs(README) : 기능 구현 반영

2c41e59

refactor(utils): splitter, parser를 utils 패키지로 이동하여 패키지 구조 개선

3776796

refactor(view): InputView, OutputView를 view 패키지로 이동하여 입출력 책임 분리

6c1a374

refactor(domain): 도메인 관련 클래스를 domain 패키지로 이동하여 패키지 구조 정리

53fc2e4

feat(Number) : 음수 값에 대한 유효성 검사를 추가하고 에러 메시지를 개선

7dd86ad

test(NumberTest) : 음수 값 예외 테스트 추가 및 DisplayName 수정

8b64f44

refactor(Numbers) : NumberTokens 클래스를 Numbers로 변경하고 음수 검증 책임을 일원화

56940b5

docs(README) : Step4 요구사항 작성

3ae1805

chore(build.gradle) : AssertJ 의존성 추가

5659694

docs(README) : 의존성 추가 작업 반영

d8f5182

refactor(CalculatorTest) : AssertJ로 테스트 검증 방식 통일

cb64fa7

refactor(CalculatorTest) : AssertJ로 테스트 검증 방식 통일

876f38b

refactor(CustomDelimiterRegisterTest) : AssertJ 적용

3f668c9

refactor(DelimitersTest) : AssertJ 적용

504a9a2

refactor(NumbersTest) : AssertJ 적용

bbfee4c

refactor(NumberTest) : AssertJ 적용

f717a2b

refactor(StringCalculatorTest) : AssertJ 적용

6194d53

refactor(CustomDelimiterParserTest) : AssertJ 적용

42d9f76

refactor(ExpressionSplitterTest) : AssertJ 적용

d8929e8

refactor(NumberParserTest) : AssertJ 적용

2d3e456

refactor(InputViewTest) : AssertJ 적용

7a7d00a

refactor(OutputViewTest) : AssertJ 적용

c53caac

docs(README) : AssertJ 적용 완료 반영

29b269f

Ohzzi suggested changes Apr 30, 2025

View reviewed changes

jsoonworld commented May 1, 2025

View reviewed changes

refactor(step4) : 코드 리뷰 반영

bdd834c

Ohzzi approved these changes May 3, 2025

View reviewed changes

boorownie merged commit 1fe147f into next-step:jsoonworld May 3, 2025

		@@ -0,0 +1,14 @@
		package domain;

		public class CustomDelimiterRegister {

		@@ -0,0 +1,30 @@
		package utils;

		public class CustomDelimiterParser {


		private final Calculator calculator = new Calculator();

		@ParameterizedTest


		private static final String ERROR_NEGATIVE_NUMBER = "[ERROR] 숫자는 0 이상이어야 합니다.";

		private final int positiveNumber;

[4단계 - 리팩터링] 권장순 과제 제출합니다. #91

[4단계 - 리팩터링] 권장순 과제 제출합니다. #91

Uh oh!

Conversation

jsoonworld commented Apr 30, 2025

💬 4단계 - AssertJ 기반 테스트 리팩터링 중 고민된 부분을 공유드립니다!

1. assertThat(expected).isEqualTo(actual) vs assertThat(actual).isEqualTo(expected)?

2. isZero() 같은 단축 표현을 언제 활용할지

3. 예외 테스트: assertThatThrownBy 사용 시 한 줄로 끝내는 것이 좋은가?

4. contains vs isEqualTo : 문자열 비교에서의 의도 표현

5. AssertJ 전환의 기준: 모든 테스트를 바꾸는 것이 맞는가?

Uh oh!

Ohzzi left a comment

Choose a reason for hiding this comment

StringCalculator의 책임 범위가 너무 넓은가?

CustomDelimiterParser의 위치와 역할

Delimiters에서 구분자를 추가하는 방식

Number가 유효성 검증 책임을 갖는 것이 좋은가?

입력값 분리 흐름이 적절한가?

assertThat(expected).isEqualTo(actual) vs assertThat(actual).isEqualTo(expected)?

isZero() 같은 단축 표현을 언제 활용할지

예외 테스트: assertThatThrownBy 사용 시 한 줄로 끝내는 것이 좋은가?

contains vs isEqualTo : 문자열 비교에서의 의도 표현

AssertJ 전환의 기준: 모든 테스트를 바꾸는 것이 맞는가?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1. `assertThat(expected).isEqualTo(actual)` vs `assertThat(actual).isEqualTo(expected)`?

2. `isZero()` 같은 단축 표현을 언제 활용할지

3. 예외 테스트: `assertThatThrownBy` 사용 시 한 줄로 끝내는 것이 좋은가?

4. `contains` vs `isEqualTo` : 문자열 비교에서의 의도 표현

jsoonworld left a comment •

edited

Loading