-
Notifications
You must be signed in to change notification settings - Fork 82
[4단계 - 리팩터링] 권장순 과제 제출합니다. #91
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
Changes from all commits
f761dc6
86bcbed
81fb8ba
5757e1b
91c60ea
393680e
25c1699
9baf34e
d003658
2cceffb
23eb753
c9987be
95a2615
c700c13
d3e0005
80829b8
fd6099c
95b22bf
d302d13
92c7190
18281a9
ad8e38f
30da163
112be7e
7708187
e9b2dd2
264d73a
1197396
5c51178
f4b698d
43a7301
3cae316
06f5f7c
3fcf1a5
dc457d4
399f221
12a0e4f
4fae504
541702d
40d57c2
5687581
c45a334
1db605b
2c41e59
3776796
6c1a374
53fc2e4
7dd86ad
8b64f44
56940b5
3ae1805
5659694
d8f5182
cb64fa7
876f38b
3f668c9
504a9a2
bbfee4c
f717a2b
6194d53
42d9f76
d8929e8
2d3e456
7a7d00a
c53caac
29b269f
bdd834c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,41 @@ | ||
# java-calculator | ||
# 기능 및 프로그래밍 요구 사항 | ||
|
||
# Step4 | ||
|
||
- [x] build.gradle에 AssertJ 의존성을 추가한다. | ||
- [x] 기존 JUnit5로 작성되어 있던 단위 테스트를 AssertJ로 리팩터링한다. | ||
- [x] JUnit5에서 제공하는 기능과 AssertJ에서 제공하는 기능을 사용해보고, 어떠한 차이가 있는지 경험한다. | ||
- [x] 메인 메서드는 만들지 않는다. | ||
|
||
# Step3 | ||
- [x] 쉼표(`,`) 또는 콜론(`:`)을 구분자로 가지는 문자열을 전달하면, 구분자를 기준으로 분리한 숫자들의 합을 반환해야 한다. | ||
- 예시: | ||
- "" → 0 | ||
- "1,2" → 3 | ||
- "1,2,3" → 6 | ||
- "1,2:3" → 6 | ||
- [x] 입력 문자열을 받아 처리해야 한다. | ||
- [x] 입력받은 문자열을 쉼표(`,`) 또는 콜론(`:`)을 기준으로 분리할 수 있어야 한다. | ||
- [x] 분리한 숫자들을 합산하여 반환해야 한다. | ||
- [x] 기본 구분자(쉼표, 콜론) 외에 **커스텀 구분자**를 지정할 수 있어야 한다. | ||
- [x] 쉼표, 콜론은 기본 구분자이다. | ||
- [x] 커스텀 구분자를 기본 구분자에 **추가 등록**할 수 있어야 한다. | ||
- [x] 커스텀 구분자는 문자열 앞부분 `"//"`와 `"\n"` 사이에 위치한 문자를 추출하여 등록할 수 있어야 한다. | ||
- [x] 문자열에 숫자가 아닌 값이나 음수가 포함되면 `RuntimeException`을 발생시켜야 한다. | ||
- [x] 구현한 문자열 계산기가 예상한대로 동작하는지 Junit5를 활용하여 테스트를 자동화한다. | ||
- [x] 조금 더 복잡한 도메인을 대상으로 테스트를 작성하는 경험을 해본다. | ||
- [x] 메인 메서드는 만들지 않는다. | ||
|
||
|
||
# Step2 | ||
- [x] 구현한 초간단 계산기가 예상한대로 동작하는지 Junit5를 활용하여 테스트를 자동화한다. | ||
- [x] 메인 메서드 없이 동작을 검증하는 경험을 해본다. | ||
- [x] 메인 메서드는 만들지 않는다. | ||
|
||
# Step1 | ||
- [x] 인자 2개를 받아 사칙연산을 할 수 있는 계산기를 구현한다. | ||
- [x] 사칙연산과 매칭되는 4개의 메서드를 제공한다. | ||
- [x] 계산된 결과는 정수를 반환한다. | ||
- [x] 메인 메서드는 만들지 않는다.** | ||
|
||
|
||
계산기 미션 저장소 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package domain; | ||
|
||
public class Calculator { | ||
|
||
private static final String DIVIDE_BY_ZERO_ERROR_MESSAGE = "[ERROR] 0으로 나눌 수 없습니다."; | ||
|
||
public int add(int firstNumber, int secondNumber) { | ||
return firstNumber + secondNumber; | ||
} | ||
|
||
public int subtract(int firstNumber, int secondNumber) { | ||
return firstNumber - secondNumber; | ||
} | ||
|
||
public int multiply(int firstNumber, int secondNumber) { | ||
return firstNumber * secondNumber; | ||
} | ||
|
||
public int divide(int firstNumber, int secondNumber) { | ||
if (secondNumber == 0) { | ||
throw new IllegalArgumentException(DIVIDE_BY_ZERO_ERROR_MESSAGE); | ||
} | ||
return firstNumber / secondNumber; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package domain; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
public class Delimiters { | ||
|
||
private static final String COMMA = ","; | ||
private static final String COLON = ":"; | ||
private static final List<String> DEFAULT_DELIMITERS = List.of(COMMA, COLON); | ||
|
||
private final List<String> delimiters = new ArrayList<>(DEFAULT_DELIMITERS); | ||
|
||
public void registerCustomDelimiter(String customDelimiter) { | ||
validate(customDelimiter); | ||
delimiters.add(customDelimiter); | ||
} | ||
|
||
public List<String> getDelimiters() { | ||
return new ArrayList<>(delimiters); | ||
} | ||
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delimiters 라는 필드를 반환하지 않고 새 리스트를 반환하는 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오! 그 부분은 저도 한 번 고민했던 지점인데요 😅 차라리 List.copyOf()나 Collections.unmodifiableList() 같은 방식이 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new ArrayList나 List.copyOf나 큰 차이는 없습니다. 사실 컬렉션을 getter로 반환할 때 얼마나 방어적으로 코딩해야 하는가에 대해서는 사람마다 의견이 다른데요, 장순님께서 다만 외부에서 수정을 아예 못하게 하고 싶다면 저는 unmodifiableList를 사용하긴 할 것 같아요. 이건 취향이지만, 어쨌든 방어적 복사를 통해 참조가 끊긴 새로운 객체를 반환하더라도 복사된 새로운 컬렉션은 다시 가변인거잖아요? 가변 컬렉션을 최대한 안쓰겠다! 하는 관점에서는 unmodifiableList가 좋은 것 같아요. 그런데 사실 unmodifiableList도 불변이라고 볼 수는 없다는 점에 유의해야 합니다. 이 부분은 개인적으로 자료를 찾아보고 테스트를 통해 왜 unmodifiableList로 반환한 리스트는 불변이 아닌가 찾아보시면 도움이 될 것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이 말을 듣고 나니 다시 한번 고민하게 되네요! 결국 완전한 불변이 보장되지 않는다면, 지금처럼 복잡도를 높여가며 방어적 코드를 작성하는 게 정말 타당한가? 라는 의문이 들었어요. |
||
|
||
private void validate(String customDelimiter) { | ||
validateNotBlank(customDelimiter); | ||
validateNotDuplicate(customDelimiter); | ||
} | ||
|
||
private void validateNotBlank(String customDelimiter) { | ||
if (customDelimiter == null || customDelimiter.isBlank()) { | ||
throw new IllegalArgumentException("[ERROR] 커스텀 구분자는 비어 있을 수 없습니다."); | ||
} | ||
} | ||
|
||
private void validateNotDuplicate(String customDelimiter) { | ||
if (delimiters.contains(customDelimiter)) { | ||
throw new IllegalArgumentException("[ERROR] 이미 등록된 구분자입니다."); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
package domain; | ||
|
||
import java.util.Objects; | ||
|
||
public class PositiveNumber { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 요건 코드 구현사항하고는 다른 문제지만, Positive != NonNegative 입니다! PositiveNumber 라는 클래스라면 0을 포함할 수 없어요!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NonNegativeNumber 라고 해도 되고, 아예 0 이상이라는 의미를 담기보단 계산기의 계산 대상이라는 느낌의 네이밍으로 가도 괜찮을 것 같아요! 참고로 네이밍에는 정답은 없습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 피드백 감사합니다! 🙏 Positive != NonNegative라는 점 명확히 인지하겠습니다! 의견 주셔서 감사합니다 :) |
||
|
||
private static final String ERROR_NEGATIVE_NUMBER = "[ERROR] 숫자는 0 이상이어야 합니다."; | ||
|
||
private final int positiveNumber; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PositiveNumber 안에 value 값으로 positiveNumber 이름이 또 들어가니까 어색한 것 같아요! 보통 한 값을 그대로 감싸는 Value Object의 경우 내부 value 값의 이름은 value 라고 짓는 경우가 많기는 합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 피드백 감사합니다! 🙏 사용자 입장에서 봤을 때 PositiveNumber 안에 positiveNumber라는 네이밍이 연속적으로 나오는 게 어색하다는 점 공감됩니다. 의견 감사드려요 :) |
||
|
||
public PositiveNumber(int positiveNumber) { | ||
validateNonNegative(positiveNumber); | ||
this.positiveNumber = positiveNumber; | ||
} | ||
|
||
private void validateNonNegative(int positiveNumber) { | ||
if (positiveNumber < 0) { | ||
throw new IllegalArgumentException(ERROR_NEGATIVE_NUMBER); | ||
} | ||
} | ||
|
||
public int getPositiveNumber() { | ||
return positiveNumber; | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (o == null || getClass() != o.getClass()) return false; | ||
PositiveNumber that = (PositiveNumber) o; | ||
return positiveNumber == that.positiveNumber; | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(positiveNumber); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return String.valueOf(positiveNumber); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package domain; | ||
|
||
import utils.NumberParser; | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class PositiveNumbers { | ||
|
||
private final List<PositiveNumber> positiveNumbers; | ||
|
||
public PositiveNumbers(List<String> tokens) { | ||
this.positiveNumbers = tokens.stream() | ||
.map(NumberParser::parse) | ||
.collect(Collectors.toCollection(ArrayList::new)); | ||
} | ||
|
||
public int sum() { | ||
return positiveNumbers.stream() | ||
.mapToInt(PositiveNumber::getPositiveNumber) | ||
.sum(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package domain; | ||
|
||
import utils.ExpressionSplitter; | ||
|
||
import java.util.List; | ||
|
||
public class StringCalculator { | ||
|
||
private final Delimiters delimiters; | ||
private final ExpressionSplitter splitter; | ||
|
||
public StringCalculator() { | ||
this.delimiters = new Delimiters(); | ||
this.splitter = new ExpressionSplitter(delimiters); | ||
} | ||
|
||
public int calculateSum(String expression) { | ||
if (isEmpty(expression)) { | ||
return 0; | ||
} | ||
|
||
String numbersExpression = extract(expression); | ||
List<String> tokens = splitter.split(numbersExpression); | ||
PositiveNumbers positiveNumbers = new PositiveNumbers(tokens); | ||
|
||
return positiveNumbers.sum(); | ||
} | ||
|
||
private boolean isEmpty(String expression) { | ||
return expression == null || expression.isBlank(); | ||
} | ||
|
||
private String extract(String expression) { | ||
final String prefix = "//"; | ||
final String suffix = "\n"; | ||
if (!expression.startsWith(prefix)) { | ||
return expression; | ||
} | ||
int endIndex = expression.indexOf(suffix); | ||
if (endIndex == -1) { | ||
throw new IllegalArgumentException("[ERROR] 커스텀 구분자 형식이 잘못되었습니다."); | ||
} | ||
String customDelimiter = expression.substring(prefix.length(), endIndex); | ||
delimiters.registerCustomDelimiter(customDelimiter); | ||
return expression.substring(endIndex + 1); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
package utils; | ||
|
||
import domain.Delimiters; | ||
|
||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.regex.Pattern; | ||
import java.util.stream.Collectors; | ||
|
||
public class ExpressionSplitter { | ||
|
||
private final Delimiters delimiters; | ||
|
||
public ExpressionSplitter(Delimiters delimiters) { | ||
this.delimiters = delimiters; | ||
} | ||
|
||
public List<String> split(String expression) { | ||
String regex = delimiters.getDelimiters().stream() | ||
.map(Pattern::quote) | ||
.collect(Collectors.joining("|")); | ||
return Arrays.asList(expression.split(regex)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package utils; | ||
|
||
import domain.PositiveNumber; | ||
|
||
public class NumberParser { | ||
|
||
private static final String ERROR_NOT_A_NUMBER = "[ERROR] 입력은 숫자여야 합니다."; | ||
|
||
public static PositiveNumber parse(String input) { | ||
try { | ||
int number = Integer.parseInt(input); | ||
return new PositiveNumber(number); | ||
} catch (NumberFormatException e) { | ||
throw new IllegalArgumentException(ERROR_NOT_A_NUMBER); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
package domain; | ||
|
||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.CsvSource; | ||
|
||
import static org.assertj.core.api.Assertions.*; | ||
|
||
class CalculatorTest { | ||
|
||
private final Calculator calculator = new Calculator(); | ||
|
||
@ParameterizedTest | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 감사합니다~! ㅎㅎ |
||
@CsvSource({ | ||
"1, 2, 3", | ||
"3, 5, 8", | ||
"-1, -2, -3", | ||
"-5, 5, 0" | ||
}) | ||
@DisplayName("덧셈: 두 수를 더한 결과를 반환한다.") | ||
void addMethod(int firstNumber, int secondNumber, int expected) { | ||
int result = calculator.add(firstNumber, secondNumber); | ||
assertThat(expected).isEqualTo(result); | ||
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({ | ||
"5, 2, 3", | ||
"10, 5, 5", | ||
"0, 0, 0", | ||
"-5, -5, 0" | ||
}) | ||
@DisplayName("뺄셈: 두 수를 뺀 결과를 반환한다.") | ||
void subtractMethod(int firstNumber, int secondNumber, int expected) { | ||
int result = calculator.subtract(firstNumber, secondNumber); | ||
assertThat(expected).isEqualTo(result); | ||
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({ | ||
"2, 3, 6", | ||
"-2, 3, -6", | ||
"0, 5, 0", | ||
"-3, -3, 9" | ||
}) | ||
@DisplayName("곱셈: 두 수를 곱한 결과를 반환한다.") | ||
void multiplyMethod(int firstNumber, int secondNumber, int expected) { | ||
int result = calculator.multiply(firstNumber, secondNumber); | ||
assertThat(expected).isEqualTo(result); | ||
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({ | ||
"6, 3, 2", | ||
"9, 3, 3", | ||
"-9, 3, -3", | ||
"10, -2, -5" | ||
}) | ||
@DisplayName("나눗셈: 두 수를 나눈 결과를 반환한다.") | ||
void divideMethod(int firstNumber, int secondNumber, int expected) { | ||
int result = calculator.divide(firstNumber, secondNumber); | ||
assertThat(expected).isEqualTo(result); | ||
} | ||
|
||
@ParameterizedTest | ||
@CsvSource({ | ||
"5, 0", | ||
"0, 0", | ||
"-3, 0" | ||
}) | ||
@DisplayName("나눗셈: 0으로 나누면 IllegalArgumentException이 발생한다.") | ||
void divideByZeroException(int firstNumber, int secondNumber) { | ||
assertThatThrownBy(() -> calculator.divide(firstNumber, secondNumber)) | ||
.isInstanceOf(IllegalArgumentException.class); | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 이미 존재하는 문자열을 구분자로 추가한다면 어떻게 되나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오..! 정말 그렇네요.
이미 존재하는 구분자를 다시 추가할 경우 중복된 delimiter가 생기게 되는데,
현재 로직에서는 이를 막을 수 있는 제약이나 검증이 전혀 없는 상태라 예상치 못한 동작이 생길 여지가 충분히 있는 것 같아요.
이 부분에 대해서는 고려가 부족했던 것 같습니다.
커스텀 구분자를 받는 로직이다 보니, 중복 여부나 유효성 검사에 좀 더 민감하게 대응했어야 했는데,
단순히 리스트에 추가하는 흐름만 보고 지나친 것 같아요 😅
말씀 주신 피드백을 바탕으로,
검증 로직을 추가해보겠습니다!
감사합니다 🙏