[Step2] 문자열 덧셈 계산기 #1578

LenKIM · 2021-05-21T17:36:25Z

안녕하세요.

문자열 덧셈 계산기 코드 리뷰 부탁드립니다.

AddCalculatorModel 가 주요 코드입니다.

어떠한 피드백도 감사합니다😃

kyucumber

안녕하세요

몇가지 코멘트 남겨두었으니 확인 부탁드립니다.

kyucumber · 2021-05-22T14:34:20Z

src/main/java/calculator/AddCalculatorModel.java

+	public String userInput() {
+		return userInput;
+	}


외부 클래스에서는 사용되어지고 있지 않은 것 같습니다.

별도의 userInput 메소드 정의 없이 내부 인스턴스 변수에 this.userInput과 같이 접근할 순 없을까요?

this.userInput 으로 접근가능합니다.

코딩하면서 위와같이 작성한 이유는, AddCalculatorModel은 자료구조인가? 객체인가? 를 생각해보았습니다. 결론은, 비지니스로직을 갖기 때문에 객체라는 생각이 들었습니다.

만약, Stack, Queue 와 같이 자료구조 라면, this.userInput와 같이 접근하는게 맞지만, userInput이 하나의 자료구조이고, AddCalculatorModel은 비지니스 로직을 갖는 객체라 판단하기 때문에, 위와 같이 코딩했습니다.

그러나, 그렇게 생각했음에도 불구하고, this.sum 변수의 경우에는 바로 접근을 했네요.

아고고.. 이런게 바로 언행일치일까요...

kyucumber · 2021-05-22T14:36:40Z

src/main/java/calculator/AddCalculatorModel.java

+	private final String userInput;
+
+	private long sum = 0L;
+
+	public AddCalculatorModel(String userInput) {
+		this.userInput = userInput;
+	}


입력값을 인스턴스 변수 형태로 저장하고 있을 필요는 없을 것 같아요.

크게 상관은 없겠지만 인스턴스 변수로 가지는 것이 아닌 execute 메소드의 파라미터로 전달되어도 충분하지 않을까요?

맞습니다. 코드 리팩토링하는 과정에서 변경될 수 있는 사항이긴합니다.

의도는 UserInput 이라는 변수가 추후 다른 곳에서도 사용될 수 있을 것이라 판단되어 생성자 변수로 놓았던 것으로 보입니다. 물론 테스트 코드를 작성하면서도 쉽게 결과를 도출하려는 의도도 있었습니다.

그러나 결과물로 나온 코드는 execute() 메소드 한 곳에서만 사용되기 때문에 수정할 필요가 있겠네요!

그리고 이렇게 되면 this.userInput과 같은 클래스 변수가 아예 사라지니- 이 방식이 코드가 더욱 깔끔해보이네요!

kyucumber · 2021-05-22T14:37:38Z

src/main/java/calculator/AddCalculatorModel.java

+	}
+
+	public long execute() {
+		if (isBlank(userInput())) {


위에 남긴 리뷰와 이어지는 부분이네요.
해당 클래스 내부에서는 아래와 같이 사용해도 무방할 것 같습니다.

Suggested change

if (isBlank(userInput())) {

if (isBlank(this.userInput)) {

kyucumber · 2021-05-22T14:39:55Z

src/main/java/calculator/NumberUtils.java

+
+	public static void requireNumber(char aChar) {
+		if (aChar == '-' || Character.isAlphabetic(aChar)) {
+			throw new RuntimeException();


어떤 이유로 예외가 발생한건지 메시지를 남겨주시면 좋을 것 같네요.

요구사항에는 RuntimeException을 사용하라고 되어 있지만 IllegalArgumentException 등의 구현체를 사용해주시면 좋을 것 같아요.

네네 👍 좋아요. RuntimeException 이라는 요구조건에 벗어나질 못했네요.

kyucumber · 2021-05-22T14:48:19Z

src/main/java/calculator/NumberUtils.java

+	public static String[] requireNumber(String[] strings) {
+		for (String s : strings) {
+			requireNumber(s);
+		}
+		return strings;
+	}
+
+	public static String requireNumber(String string) {
+		for (char aChar : string.toCharArray()) {
+			requireNumber(aChar);
+		}
+		return string;
+	}
+
+	public static void requireNumber(char aChar) {
+		if (aChar == '-' || Character.isAlphabetic(aChar)) {
+			throw new RuntimeException();


Character 단위까지 꺼내서 비교할 필요가 있을까요?

커스텀 구분자, 디폴트 구분자로 나누어진 시점에 이미 숫자가 아니면 잘못된 케이스로 분류해야 할 것 같습니다.

혹시, 코드를 어떻게 수정하면 좋을까요? '구분자로 나누어진 시점에 숫자가 아니다' 라는 것을 정규표현식과 같은 방식을 사용하는 것을 말씀하시는 걸까요?

정규표현식을 활용해서 조금 더 신뢰도 있는 코드가 되도록 수정하겠습니다 : )

Character가 아니라 나누어진 String이 숫자인지 검증해도 괜찮지 않을까요?

맞습니다. 해당 코드를 곰곰히 생각해보니- 버그도 숨겨져 있더라구요. String 이 숫자인지 판단하는 로직을 좀더 단순화 시킬수 있었습니다.

kyucumber · 2021-05-22T14:52:54Z

src/main/java/calculator/StringUtils.java

+	public static String[] getSplit(String s, String regex) {
+		return s.split(regex);
+	}


이 부분을 메소드로 추출할 필요가 있을까요?
오히려 추출된 메소드의 코드 수가 더 많습니다.

s.split(regex) getSplit(s, regex)

코드 수가 더 많아지네요 😆
가독성있는 코드를 추구하려다 보니- 오버엔지니어링이 된 느낌이네요. 해당 코드는 수정하겠습니다.

kyucumber · 2021-05-22T14:55:38Z

src/main/java/calculator/DelimiterMatcher.java

+	private boolean hasCustomDelimiter = false;
+
+	private DelimiterMatcher(Matcher matcher) {
+		requireNonNull(matcher);
+		this.matcher = matcher;
+	}
+
+	public static DelimiterMatcher create(String userInput, Pattern compile) {
+		return new DelimiterMatcher(compile.matcher(userInput));
+	}
+
+	public boolean hasCustomDelimiter() {


hasCustomDelimiter 메소드 호출을 누락하면 hasCustomDelimiter 인스턴스 변수는 항상 false가 되겠네요.

생성과 동시에 체크 후 할당해줄 순 없을까요?

저도 이 부분에 대해서 고민했습니다. wrapper Class DelimiterMatcher 내에서 함수를 나누지 않고 처리하는 방향으로 수정하겠습니다.

kyucumber · 2021-05-22T14:57:08Z

src/main/java/calculator/DelimiterMatcher.java

+		if (hasCustomDelimiter) {
+			return matcher.group(1);
+		}
+		return "";


여기서 "" 와 같이 아무 Delimiter도 반환하지 않는 경우는 어떤 경우일까요?

hasCustomDelimiter가 true인 경우만 getDelimiter가 호출되므로 도달할 수 없는 코드로 보입니다.

혼란을 줄 수 있으니 예외를 던지거나 다른 방법으로 변경해주시면 어떨까요?

네, 좋습니다 : ) 다른 방법으로 변경해보겠습니다.

kyucumber · 2021-05-22T14:58:24Z

src/main/java/calculator/AddCalculatorModel.java

+			return 0L;
+		}
+
+		DelimiterMatcher matcher = DelimiterMatcher.create(userInput(), Pattern.compile(CUSTOM_DELIMITER_REGEX));


CUSTOM_DELIMITER_REGEX 값이 바뀌지 않으므로 Pattern.compile의 결과 또한 상수 형태로 사용할 수 있지 않을까요?

네 맞습니다 : ) 크게 생각하지 고민하지 않았던 부분입니다. 변경하도록 하겠습니다!

kyucumber · 2021-05-22T15:00:22Z

src/main/java/calculator/AddCalculatorModel.java

+	private long calculate(String[] strings) {
+		for (String s : strings) {
+			this.sum += Long.parseLong(s);
+		}
+		return sum;
+	}


requireNumber에서 체크가 되었다곤 하지만 여전히 파라미터로 String이 남아있습니다.

다른 메소드를 추가했을 때 이 메소드를 그대로 쓰면 예외가 발생하겠네요.

파라미터로 String이 아닌 숫자를 받을 순 없을까요?

좋은 의견 감사합니다. 메소드를 다시 보니, Long.pareLong 이 어색하네요. 해당 코드는 수정하겠습니다.

LenKIM · 2021-05-23T00:55:57Z

상세한 피드백 감사합니다 👍 해당 코드는 수정후 피드백 재요청하겠습니다.

- requiredNumber > validateString 수정 - 문자 단위가 아닌, 문자열 단위로 체크되도록 수정 - CustomDelimiterMatcher 이름 변경 및 내부 코드 변경 - AddCalculatorModel 내 불필요한 클래스 변수 제거

kyucumber

피드백 잘 반영해주셨습니다.

남겨둔 코멘트 확인 부탁드립니다.

다음 단계 진행을 위해 머지할게요.

#1578 (comment)

kyucumber · 2021-05-23T12:24:01Z

src/main/java/calculator/AddCalculatorModel.java

+	private long calculate(Long[] longs) {
+		for (Long l : longs) {
+			this.sum += l;
 		}
 		return sum;


지금과 같은 복잡도에서는 크게 상관은 없겠지만 List 등을 활용하면 어떨까요?

List에서 제공하는 내장 함수나 Stream을 활용하면 좋을 것 같네요.

다음 단계부터는 배열의 사용을 지양하고 List를 사용해보시면 좋을 것 같습니다.

네 👍 피드백 감사합니다.

LenKIM added 9 commits May 22, 2021 03:03

docs: 문자열 덧셈 계산기 기능요구사항 정의

9b31a09

feat: 컴마(,)구분자로 입력한 경우의 테스트 코드 추가

58b1533

refactor(Main): 불필요한 사용자 UI 제거

ac8b3f6

refactor: 클래스 이름변경(CalculatorModel > AddCalculatorModel)

b924d06

refactor: 메소드(calculate) 추출

c4424e9

feat: “//”와 “\n” 문자 사이에 커스텀 구분자 관련 테스트 코드 추가

9c1ac70

refactor: 메소드 시그니쳐 변수명 변경

d2f3cbe

refactor: polishing inline variable

172a6b4

refactor: 테스트 코드 간결하게 수정

fb6735a

LenKIM force-pushed the step2 branch from e6fcaa0 to fb6735a Compare May 21, 2021 18:07

style: 불필요한 개행 문자 제거

841340f

kyucumber requested changes May 22, 2021

View reviewed changes

refactor:코드 피드백 반영

7091907

- requiredNumber > validateString 수정 - 문자 단위가 아닌, 문자열 단위로 체크되도록 수정 - CustomDelimiterMatcher 이름 변경 및 내부 코드 변경 - AddCalculatorModel 내 불필요한 클래스 변수 제거

LenKIM requested a review from kyucumber May 23, 2021 02:09

kyucumber approved these changes May 23, 2021

View reviewed changes

kyucumber merged commit 2a8e097 into next-step:lenkim May 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Step2] 문자열 덧셈 계산기 #1578

[Step2] 문자열 덧셈 계산기 #1578

LenKIM commented May 21, 2021 •

edited

kyucumber left a comment

kyucumber May 22, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

LenKIM May 23, 2021 •

edited

kyucumber May 22, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

LenKIM May 23, 2021

kyucumber May 23, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

kyucumber May 22, 2021

LenKIM May 23, 2021

LenKIM commented May 23, 2021

kyucumber left a comment •

edited

kyucumber May 23, 2021

LenKIM May 23, 2021

[Step2] 문자열 덧셈 계산기 #1578

[Step2] 문자열 덧셈 계산기 #1578

Conversation

LenKIM commented May 21, 2021 • edited

kyucumber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenKIM May 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenKIM commented May 23, 2021

kyucumber left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenKIM commented May 21, 2021 •

edited

LenKIM May 23, 2021 •

edited

kyucumber left a comment •

edited