[문자열 계산기] 김난슬 미션 제출합니다. by seulnan · Pull Request #8 · Java-JavaScript-Language-Stuty/java-calculator-7

seulnan · 2025-02-07T14:34:10Z

신경써서 구현한 부분

자바를 잘 몰라도 flow가 이해되도록 간단한 MVC 패턴을 도입했습니다. (너무 분리하는 것도 그닥 좋지않다고 생각하기때문입니다.)
최대한 주어진 테스트 코드를 변형하지않고 활용하려했습니다.
기능별로 단위테스트 & 통합테스트코드를 작성하여 리소스낭비를 최대한 막고자했습니다.

피드백이 필요한 부분

메서드의 파리미터의 네이밍의 의미가 중복되어 걱정인데 마땅히 해결방법이 떠오르지않아 다른분들의 의견을 묻고싶습니다.
- 현재는 userInput, input, num이런식으로 되어있는데 의미가 분리되지않아 맘에 들지않습니다.
- 이외에도 직관적이지않은 네이밍이 있다면 피드백 부탁드립니다!
불필요한 인라인 코드, 또는 불필요한 변수선언이 있다면 알려주세요.
예외처리 메시지 및 흐름이 일관성이 있는지 확인해주시면 감사하겠습니다.
코드 리팩토링 과정에서 성능이나 유지보수 측면에서 개선할 점이 있는지 많은 조언 부탁드립니다.

고려한 예외

음수 입력 시
잘못된 숫자 형식 입력 시 예외 발생
커스텀 구분자 (//;\n) 사용 시 구분자 이후 숫자가 없을 시

Added a list of features to be implemented in README.md. This serves as a roadmap for the upcoming development tasks.

…ult delimiters Implemented sum calculation using split and Java Streams. - Supported default delimiters (comma, colon) - Empty input returns 0 - Unit tests added to verify expected behavior.

…ault and custom delimiters - Supported default delimiters (comma, colon) - Added support for custom delimiters (e.g., `//;\n1;2;3`) - Unit tests added to verify expected behavior

- Implemented exception handling for negative numbers and invalid input formats - Throws `IllegalArgumentException` when an invalid input is encountered - Ensured proper application termination upon encountering an exception - Verified functionality using provided integration test code

moongua404

첫 주차부터 MVC패턴, Junit, stream API 등등 많은 부분들을 적용시킨 부분이 눈에 띄는 것 같습니다. 개인적으로는 다양한 예외를 구체적으로 처리한 부분이 인상깊었습니다! 저도 예외처리를 좀 더 신경써야 할 것 같아요...ㅎㅎ
1주차 미션 너무 수고많으셨습니다!

moongua404 · 2025-02-08T06:33:09Z

src/main/java/calculator/controller/CalculatorController.java

+import calculator.view.InputView;
+import calculator.view.OutputView;
+
+public class CalculatorController {


컨트롤러에서 뷰와 모델을 static call을 하는 이유가 있나요?
특별한 이유가 없다면 의존성을 생각해서 DI를 하는게 더 낫지 않을까요?

아하 그 방향대로 수정해보겠습니다!

moongua404 · 2025-02-08T06:36:29Z

src/main/java/calculator/model/Calculator.java

+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class Calculator {


필드가 없고 기능만 있다면 모델 보다는 service나 utils로 취급되는게 더 적절해보여요!
모델로 둔다면 모델인 만큼 상태를 저장하는 목적 등이 강조되면 좀 더 좋을 것 같아요

moongua404 · 2025-02-08T06:37:34Z

src/main/java/calculator/model/Calculator.java

+import java.util.regex.Pattern;
+
+public class Calculator {
+    public static int processSum(String numbers) {


이 메서드가 갖고있는 책임이 너무 큰 것 같습니다. 구분자를 확인 및 설정하는 것, 예외를 처리하는 것, 계산하는 것 등 세부 기능으로 메서드를 만들어 분리하는 쪽이 더 좋지 않을까요?

moongua404 · 2025-02-08T06:39:43Z

src/main/java/calculator/model/Calculator.java

+
+        if (matcher.find()) {
+            String customDelimiter = Pattern.quote(matcher.group(1));
+            delimiters = Arrays.asList(customDelimiter, ",", ":");


delimiter 필드를 재정의할 필요가 있을까요? List로 둔다면 ArrayList 내지는 LinkedList 등으로 두고 원소를 추가하는 편이 더 직관적으로 와닿을 것 같아요!

저도 원소를 추가하는 편이 좀 더 낫고 안전할 것 같아요!

동의합니다.
한 번에 사용 가능한걸 두 번에 나눌 필요는 없다고 생각합니당

오호 그 방법으로 하는게 더 가독성 좋을 것 같네용

moongua404 · 2025-02-08T06:43:57Z

src/main/java/calculator/model/Calculator.java

+                .sum();
+    }
+
+    private static int parseValidation(String num) {


밑에서 변환 예외와 음수 예외를 다루는데 제가 받아들이기에는 역할이 2개여서 가독성이 떨어지는 것 같아요. 개인적인 생각으로는 검증 역시 메서드를 나눠도 좋을 것 같아요.
그리고 검증을 모델 내부에 두신 이유도 궁금합니다!ㅎㅎ

검증이랑 예외처리를 따로 분리하고싶었으나...시간 부족이슈로.. 구현과 테스트 정상작동에 집중했습니다. 수정해보겠습니다!

src/main/java/calculator/view/OutputView.java

Regyung

리뷰 남기면서 저는 아직 한참 멀었다는 걸 깨달았습니다...

Regyung · 2025-02-09T14:08:19Z

src/main/java/calculator/model/Calculator.java

+                .sum();
+    }
+
+    private static int parseValidation(String num) {


어떠한 기능만 수행해서 return해주는 메서드의 파라미터는 첫 글자만 남겨서 네이밍해도 괜찮을 것 같아요.
ex) number -> n, string -> s

제안해주신 방식도 간결한 코드에서 효과적일 수 있다고도 생각합니다. 다만, 네이밍이 짧아지면 가독성이 떨어지고 유지보수할 때 혼란이 될 수도 있다고 생각해서요 그래도 현록님이 이 방식이 더 괜찮다고 생각하시는 이유가 있을까용? 생각하시는 더 좋은 규칙이 있다면 같이 얘기해보고싶어요! @Regyung

Regyung · 2025-02-09T14:29:10Z

src/test/java/calculator/CustomCalculatorTest.java

+    void 기본_구분자_혼합사용_테스트() {
+        assertThat(Calculator.processSum("//;\\n1;2,3:4")).isEqualTo(10);
+    }
+}


저도 나중에 생각난 것이지만 커스텀 구분자가 2개 이상일 수도 있을 상황이 존재할 것도 같습니다!

오오 그 예외는 생각하지 못했는데 반영해보겠습니당

Regyung · 2025-02-09T14:31:53Z

src/main/java/calculator/model/Calculator.java

+
+        if (matcher.find()) {
+            String customDelimiter = Pattern.quote(matcher.group(1));
+            delimiters = Arrays.asList(customDelimiter, ",", ":");


저도 원소를 추가하는 편이 좀 더 낫고 안전할 것 같아요!

src/test/java/calculator/BasicCalculatorTest.java

1jae2jae3jae · 2025-02-10T06:25:20Z

src/main/java/calculator/view/OutputView.java

+        System.out.println("결과 : " + result);
+    }
+    public static void printError(String message) {
+        System.out.println("오류 발생 : " + message);


저도 같은 생각입니다!

1jae2jae3jae · 2025-02-10T06:40:18Z

src/main/java/calculator/model/Calculator.java

+
+        if (matcher.find()) {
+            String customDelimiter = Pattern.quote(matcher.group(1));
+            delimiters = Arrays.asList(customDelimiter, ",", ":");


저도 추가하는 편이 더 좋은 것 같습니다!

Siul49 · 2025-02-11T13:26:52Z

src/main/java/calculator/model/Calculator.java

 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
-import java.util.stream.Collectors;



덕분에 좋은 라이브러리들 알아갑니다

Siul49 · 2025-02-11T14:05:21Z

src/main/java/calculator/model/Calculator.java

+
+        if (matcher.find()) {
+            String customDelimiter = Pattern.quote(matcher.group(1));
+            delimiters = Arrays.asList(customDelimiter, ",", ":");


동의합니다.
한 번에 사용 가능한걸 두 번에 나눌 필요는 없다고 생각합니당

Siul49 · 2025-02-11T14:33:38Z

src/test/java/calculator/CustomCalculatorTest.java

문자열 중간에 커스텀 구분자를 추가할 순 없을까요?
예) //]\n1]//[\n1,2
문제서 어느정도까지의 융통성을 요구하는지 모르겠는데 어떻게 판단하고 케이스를 짜셨나요

커스텀 구분자는 문자열 앞부분의 //와 \n 사이에 위치하는 문자를 사용한다 는 기능 요구사항을 기준으로 잡았습니다. 입력값을 커스텀 구분자로 나눈 후에도 숫자가 아닌 값이 포함될 경우, 이는 잘못된 입력이라고 판단했습니다. 따라서, 경수님이 말씀해주신 중간에 임의의 구분자를 추가하는 케이스는 예외로 처리하는 것이 더 적절하다고 보았습니다

혹시 경수님께서 중간에 커스텀 구분자를 허용해야 한다고 생각하신다면, 어떤 요구사항을 기준으로 고려하셨는지 의견을 나눠보면 좋을 것 같아요! @Siul49

Siul49 · 2025-02-11T14:35:12Z

src/test/java/calculator/BasicCalculatorTest.java

+import calculator.model.Calculator;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;


테스트 케이스를 공부하게 되었습니다 이렇게 쓰는거였군요

seulnan added 4 commits February 4, 2025 21:29

[DOCS](README): add planned feature list

502ca47

Added a list of features to be implemented in README.md. This serves as a roadmap for the upcoming development tasks.

feat(basic-delimiter-calculator): implement sum calculation with defa…

64d3c5f

…ult delimiters Implemented sum calculation using split and Java Streams. - Supported default delimiters (comma, colon) - Empty input returns 0 - Unit tests added to verify expected behavior.

feat(custom-delimiter-calculator): implement sum calculation with def…

4c04182

…ault and custom delimiters - Supported default delimiters (comma, colon) - Added support for custom delimiters (e.g., `//;\n1;2;3`) - Unit tests added to verify expected behavior

moongua404 reviewed Feb 8, 2025

View reviewed changes

Regyung reviewed Feb 9, 2025

View reviewed changes

1jae2jae3jae reviewed Feb 10, 2025

View reviewed changes

Siul49 reviewed Feb 12, 2025

View reviewed changes

Conversation

seulnan commented Feb 7, 2025

신경써서 구현한 부분

피드백이 필요한 부분

고려한 예외

Uh oh!

moongua404 left a comment

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!

Uh oh!

Regyung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seulnan Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

seulnan Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seulnan Feb 12, 2025 •

edited

Loading

seulnan Feb 12, 2025 •

edited

Loading