-
Notifications
You must be signed in to change notification settings - Fork 0
[문자열 덧셈 계산기] 이예진 미션 제출합니다. #1
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
base: main
Are you sure you want to change the base?
Conversation
Eunsaem03
left a comment
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.
안녕하세요! 1주차 고생하셨습니다 😊
제가 아직 자바가 미숙하고 요구 사항 구현하는 것도 거의 처음이라 리팩토링에 도움 되는 조언을 드리지 못해서 죄송하네요.. 😢
대신 제가 코드 읽어보면서 궁금했던 점 몇 가지 질문 남겼습니다
혹시 실례가 안 된다면 자바 공부는 어떻게 하셨는지 물어봐도 괜찮을까요? 저는 기능 목록 짜는 것부터 막혔던터라.. 코드 보고 정말 놀랐습니다 많이 배워갑니다..!
| public static Result calculate(String[] tokens) { | ||
| if (tokens == null || tokens.length == 0) { | ||
| throw new IllegalArgumentException(ExceptionMessages.INPUT_EMPTY.get()); | ||
| } | ||
|
|
||
| int sum = Arrays.stream(tokens) | ||
| .mapToInt(Calculator::parseAndValidate) | ||
| .sum(); | ||
|
|
||
| return new Result(sum); | ||
| } | ||
|
|
||
| private static int parseAndValidate(String token) { | ||
| if (token == null || token.isBlank()) { | ||
| throw new IllegalArgumentException(ExceptionMessages.INPUT_EMPTY.get()); | ||
| } |
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.
이 부분에서 왜 tokens랑 token을 둘 다 확인하는건가요?!
tokens == null || tokens.length == 0 과 token == null || token.isBlank()가 각각 어떤 것을 검사하는지 차이가 궁금합니당
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.
저는 tokens는 문자열을 split해서 숫자들을 담은 String 배열이고, token은 그 배열의 각 요소들이라고 정의했는데요,
예를 들어서 입력이 "1,2:3"이라면 split 결과는 tokens = ["1", "2", "3"], token은 "1", "2", "3"이 됩니다
코드 흐름상으로는 먼저 calculate() 메서드에서 tokens에 대한 유효성 검사를 하고, 통과가 되면 .mapToInt(Calculator::parseAndValidate)를 통해 각 요소인 token을 순회하며 parseAndValidate에 인자로 넘기게 했습니다
calculate() 메서드의 tokens == null || tokens.length == 0는 입력 자체가 null이거나 || 빈 문자열("")일 경우를 검증하도록 했습니다
parseAndValidate() 메서드의 token == null || token.isBlank()는 String 배열의 각 요소에 대한 유효성 검사인데요, 만약 tokens = ["1", "", " ", "2"]인 경우라면 token 인자로 ""나 " "이 들어왔을 때 각각 token == null, token.isBlank()에 걸려서 예외로 처리하도록 한 부분이에요
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.
둘의 차이가 뭘까 궁금했는데 세세하게 잘 구분해서 검사하신 거였네요! 람다식 축약형까지 잘 배워갑니다!
| private static final List<String> DEFAULT_DELIMITERS = List.of( | ||
| Constants.DEFAULT_DELIMITER_COMMA.get(), | ||
| Constants.DEFAULT_DELIMITER_COLON.get() | ||
| ); |
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.
List.of()는 어떨 때 사용하나요?
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.
List.of()는 불변 리스트를 만들 수 있어서 보통 읽기 전용의 리스트를 만들 때 사용합니다
배열로 List를 만드는 메서드로는 Arrays.asList()도 있지만, 이건 수정이 가능한 가변 리스트를 만드는 메서드예요
기본 구분자는 ",", ":"만 사용이 가능하고, 변경되면 안되기 때문에 기본 구분자를 담는 DEFAULT_DELIMITERS 또한 List.of()를 사용해서 불변 리스트로 만들었어요
Arrays.asList()로도 동작은 하지만, 자료구조의 선택 자체로도 불변의 의도를 표현할 수 있어서 List.of()를 사용했습니다
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.
자료구조의 선택 자체로도 불변의 의도를 표현할 수 있다 는 말이 와닿네요 저도 그렇게 의도를 가지고 선택해봐야겠습니다
| /** | ||
| * 문자열 계산기에서 사용되는 상수 모음 | ||
| */ | ||
| public enum Constants { |
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.
enum으로 이렇게 사용하신 게 되게 편해보였어요!
enum은 원하는 문자열을 저장하고 편하게 꺼내서 사용하기 위해 쓰는 건가요?
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.
enum은 1) 문자열에 의미 있는 이름을 부여해 가독성을 높이고 2) 코드 수정 시 한 곳만 수정하면 되도록, 유지보수성을 높이기 위해 사용했습니다
-
예를 들어, 구체적인 요구사항까지는 모르는 사람이 코드를 볼 때
",",":"를 보면 이 값이 어떤 역할인지 알기 어렵지만,DEFAULT_DELIMITER_COMMA,DEFAULT_DELIMITER_COLON처럼enum로 관리하면 역할이 명확히 드러나서 가독성이 높아져요 -
"음수는 입력할 수 없습니다."등의 문자열을 하드코딩하면 수정이 필요할 시 해당 리터럴이 사용된 모든 위치를 찾아서 수정해야 하는 번거로움이 생깁니다. 또한,"음수는 입력 없습니다."이런 식으로 오타가 나는 등의 일관성 문제도 생기고요. 따라서 공통으로 사용되는 리터럴들을 공통 상수로 처리하게 되면 상수를 관리하는 코드 한 곳만 수정해도 되고, 오타 등의 일관성 문제도 방지할 수 있어서 유지보수성이 높아져요
이런 이유로 하드코딩된 리터럴 대신 enum 상수를 사용했습니다
| try { | ||
| int number = Integer.parseInt(token); | ||
| if (number < 0) { | ||
| throw new IllegalArgumentException(ExceptionMessages.NEGATIVE_NUMBER.get()); | ||
| } | ||
| return number; | ||
| } catch (NumberFormatException e) { | ||
| throw new IllegalArgumentException(ExceptionMessages.INVALID_NUMBER_FORMAT.get()); |
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.
저는 try~catch 사용했을 때 오류가 나서 catch 에서 다시 throw e로 처리하면서 프로그램을 강제 종료 시켰는데 어떤 분이 return으로 메세지 출력 후 종료시키는 건 어떠냐고 해주시더라구요 이때 말하신 return이 예진님이 사용하신 것처럼 try에서 하는 게 맞나요?
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.
- 이 위치(계산 로직을 수행하는 코드)에서 예외를 어떻게 처리해야 하는가
- return으로 메시지를 출력한 후 프로그램을 종료시키는 방법이 이 코드인가, 좋은 방법인가
이 두 가지 의미의 질문이라고 해석되는데요 맞을까요?
공식 테스트는 IllegalArgumentException이 상위 계층까지 전달되어 예외가 터져야 통과하기 때문에, 그보다 하위인 계층에서 catch 후 return으로 종료하게 하면 테스트가 실패하게 돼요.
저는 Calculator 등의 Model 계층은 검증/파싱/계산의 로직만, Controller는 프로그램의 흐름 제어만 담당하도록 설계했는데요. 그래서 Model 계층에는 종료 로직(return 등)을 넣지 않았고, 발생하는 예외들을 catch로 잡아 상위 계층인 Controller로 던지고, Controller에서 try-catch를 사용하지 않아 예외를 잡지 않았고 그대로 던져지며 최종적으로 프로그램의 종료를 제어하도록 설계했습니다.
제 Controller 코드를 발췌했는데요
public void run() {
String input = readInput();
Result result = processInput(input);
printResult(result);
}
try-catch를 쓰지 않았고, 예외가 발생하면 자동으로 상위로 던져져서, 프로그램이 종료되는 구조예요
return으로 메시지를 출력한 후 종료시키는 방식은 예외가 던져지는게 아니고, 예외 메시지를 콘솔에 출력 후 main 메서드가 정상 종료되는 구조로ㅡ 예외가 던져지지 않아서 공식 테케 통과가 안될 것이라고 생각하는데요
제가 한번 이 방식으로 적용해보고 어떻게 동작했는지 확인해서 알려드릴게요!
그리고 제 생각에는 return으로 메시지를 출력 후 종료하는 방식은 로직과 출력이 분리되지 않아서 좋지 않은 설계인거 같아요ㅠ
정상 흐름과 예외 흐름이 섞여버리기 때문에요
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.
잡고 던진다는 게 어떤 뜻인가 이해가 잘 안 갔었는데 설명과 코드 같이 보면서 이해했습니다! 친절한 설명 감사드려요 😄
저는 무조건 try-catch로 잡아서 던져야 하는 줄 알았는데 아니었더라구요
말씀하신 것처럼 자동으로 상위로 던져져서 프로그램이 종료한다는 게 저희가 처리하던 예외가 RuntimeException 계열이고, 이런 런타임 예외는 throws가 없어도 자동 전파되어서 try-catch를 사용하지 않고도 자동으로 상위로 던져진 것이 맞을까요?
return으로 메시지 출력 후 종료하는 방식에 대한 생각도 잘 들었습니다! 직접 적용해보고 확인해서 알려주신다니 감사합니다 👍
|
@Eunsaem03 리뷰 감사합니다! 🙇♀️ 저도 지난주 구현을 하면서 부족함을 느껴서 오브젝트 책을 읽으면서 공부하고 있는데요, 이런 책으로 방법론 공부 + 우테코 미션을 예제로 연습 + 코드 리뷰와 다른 분들 코드 보며 공부하면 조금씩 감이 잡힐 것 같아요(저도 말만 거창하게 하는거고 사실은 너무 어렵..) 요구사항 분석, 프로젝트 구조 설계, 기능 목록 뽑기 등의 미션 푸는 과정은 제가 1주차 미션하며 정리해둔 게 있어서 조금 다듬고 디코 채널에 보내드릴게요! |
seongm1n
left a comment
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.
1주차 고생 많으셨습니다. 앞으로 계속 발전하는 모습 기대하겠습니다 👍
전반적으로 객체지향을 고민하시고 코드를 작성하시는게 느껴졌습니다.
실력도 아주 좋으신 것 같아요 🔥 남은 기간동안 코드 리뷰 주고 받으며 함께 성장했으면 좋겠습니다!
| Result result = processInput(input); | ||
| return "결과 : " + result.getValue(); | ||
| } | ||
| } No newline at end of file |
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.
이 부분에 대해서는 미처 신경쓰지 못했습니다😓
놓친 부분인데 디테일하게 검토해주시고 알려주셔서 감사합니다!
| public static Result calculate(String[] tokens) { | ||
| if (tokens == null || tokens.length == 0) { | ||
| throw new IllegalArgumentException(ExceptionMessages.INPUT_EMPTY.get()); | ||
| } | ||
|
|
||
| int sum = Arrays.stream(tokens) | ||
| .mapToInt(Calculator::parseAndValidate) | ||
| .sum(); | ||
|
|
||
| return new Result(sum); | ||
| } |
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.
여기에서 static을 사용하신 이유가 있으신가요? 저는 static은 존재만으로도 객체지향에서 많이 벗어난다고 생각합니다. 😄
정말 너무 간단하거나 변경이 없어서 static으로 사용했을 때 압도적으로 편한게 아니라면 사용을 지양하고 있어요!
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.
Calculator가 상태를 가지지 않고, 입력을 받아 단순히 계산만 수행하는 순수 함수에 가깝다고 판단해서 static으로 작성했습니다.
인스턴스 생성 없이 바로 사용 가능하게 하려는 의도로 이렇게 구현을 했습니다.
말씀하신 부분을 생각해보니 객체지향적으로는 책임을 가진 객체가 동작을 수행하는 구조가 더 바람직하다고 생각되어서 이후 확장을 고려하면 인스턴스 메서드로 전환하는 편이 맞을 것 같습니다.
좋은 지적 덕분에 구조적으로 한 단계 더 생각해보게 됐어요 감사합니다!
| /** 결과 출력 */ | ||
| private void printResult(Result result) { | ||
| outputView.printResult(result); | ||
| } |
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.
Controller에 printResult메서드가 있는게 조금 이질감이 드네요. 이 부분은 굳이 분리할 필요가 있을까요?
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.
View에 직접 접근하기보다 Controller가 최종 출력을 호출하도록 구현해 두면, 나중에 출력 방식이 달라질 때 Controller의 흐름 구조만 봐도 전체 I/O 순서를 한눈에 파악할 수 있을 것 같다고 생각해서 이렇게 분리했습니다.
하지만 다시 생각했을 때 과도하게 확장성을 고려한 것 같아 현재 단순한 콘솔 환경에서는 말씀처럼 바로 outputView.printResult(result)를 호출하는 편이 더 간결할 것 같습니다.
좋은 피드백 감사드립니다! 🙇♀️
| printResult(result); | ||
| } | ||
|
|
||
| /** 사용자 입력 */ |
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.
말씀하신 부분 공감합니다 이후 개발에 반영하도록 하겠습니다.
| /** 테스트 전용 메서드 (콘솔 I/O 배제) */ | ||
| public String testRun(String input) { | ||
| Result result = processInput(input); | ||
| return "결과 : " + result.getValue(); | ||
| } |
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.
빠르게 테스트 환경을 구성하는 과정에서 구조적으로 고려가 부족했던 부분인데, 지적 감사합니다
다음 단계에서는 I/O 추상화 → 주입 테스트 구조로 개선해보겠습니다
| @Override | ||
| public String toString() { | ||
| return "결과 : " + value; | ||
| } |
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.
getValue가 있는데 toString을 사용할 이유가 있을까요?
출력 형식의 관심사는 View가 맞는 것 같습니다!
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.
말씀하신 부분처럼 View가 출력 형식을 담당하는 것에 동의하고,
도메인 객체가 화면 표현까지 책임지는 것은 MVC 분리 원칙에 어긋날 수 있다고 공감합니다.
구현에서 toString()은 디버깅이나 테스트 로그용으로 객체의 상태를 직관적으로 확인하기 위한 보조 수단으로 두었습니다. 실제 출력 로직에서는 OutputView가 getValue()를 사용해 메시지를 구성하고 있어서, toString()이 View 책임을 침범하지는 않는 구조입니다.
다만 말씀 주신 대로 혼동을 줄이려면 이 메서드는 제거하거나, toLogString()처럼 명시적인 이름으로 바꿔서 의도를 구분하도록 하겠습니다.
sanchaehwa
left a comment
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.
기능별로 코드를 나눠 작성해주셔서, 이해하고 파악하는데 도움이 되었던 것 같아요 👍🏻
enum으로 상수값 을 처리하여 가독성을 높이는 부분이 인상 깊었습니다
예진님 코드를 통해 많이 배워갑니다 :) 1주차 수고 많으셨습니다 🙇🏻♀️
| } | ||
|
|
||
| try { | ||
| int number = Integer.parseInt(token); |
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.
Integer.parseInt는 문자열 형태로 된 숫자를 정수 타입으로 변환해주는 역할을 하기 때문에 숫자 형식이 아닌 값이 들어왔을때 NumberFormatException (숫자형 포맷 오류)가 발생할 수도 있을것 같아요
그래서 Integer.parseInt 코드를 실행하기 전에 숫자 형식인지 검증 로직이 들어가면 좋을것같아요 !
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.
try-catch 블록에서 NumberFormatException을 감싸서 IllegalArgumentException(ExceptionMessages.INVALID_NUMBER_FORMAT)으로 변환하는 방식으로 예외를 도메인 단위로 통일하려는 의도로 작성했는데요, 말씀해주신 것처럼 사전 검증을 추가하면 예외 흐름을 줄이고 가독성이 개선될 것 같습니다!
리팩토링 때 이 부분 신경 써서 개선해보겠습니다 🙏
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.
저는 성공/실패 메시지만 enum으로 처리했는데, 상수도 enum으로 정리하면 테스트나 확장할 때 좋을 것 같네요 👍🏻
|
@seongm1n 리뷰 감사합니다! 미처 생각하지 못한 부분을 꼼꼼하게 짚어주셔서 많은 도움이 되었어요. |
|
@sanchaehwa 리뷰 감사합니다 😊 |
java-calculator-precourse
문자열 덧셈 계산기
입력한 문자열에서 숫자를 추출하여 합산하는 계산기를 구현한다.
기본 구분자는 쉼표(
,)와 콜론(:)이며,사용자는
"//[구분자]\\n"형식으로 커스텀 구분자를 지정할 수 있다.잘못된 입력에 대해서는
IllegalArgumentException을 발생시키고 프로그램을 종료한다.1. 기능 구현 목록
1. 입력
camp.nextstep.edu.missionutils.Console.readLine()을 사용"1,2:3"2. 입력 검증
IllegalArgumentException"") 입력 → 정상 처리"//;\n"누락) → 예외 발생3. 구분자 파싱
"//"가 있고, 그 뒤에"\n"이 있으면 커스텀 구분자로 인식한다"//;\n1;2;3"→ 구분자:;→ 결과:6"//***\n1***2***3"→ 결과:6IllegalArgumentException//,\n,,,:) 사용 불가 → 예외 발생IllegalArgumentExceptionIllegalArgumentException4. 문자열 파싱
"a","--","-2"등 →IllegalArgumentExceptionIllegalArgumentException"1,2:3"→ 출력:["1", "2", "3"]5. 계산
IllegalArgumentException["1", "-2", "3"]→ 예외 발생["1", "a", "3"]→ 예외 발생6. 출력
"결과 : X"형식으로 출력한다"결과 : 6"2. 예외 처리 정책
1. 입력 단계
IllegalArgumentException"입력값이 존재하지 않습니다."IllegalArgumentException"커스텀 구분자 형식이 올바르지 않습니다."2. 검증 단계
IllegalArgumentException"음수는 입력할 수 없습니다."IllegalArgumentException"숫자와 구분자만 입력해주세요."3. 구분자 파싱 단계
IllegalArgumentException"커스텀 구분자 형식이 올바르지 않습니다."IllegalArgumentException"공백은 구분자로 사용할 수 없습니다."//,\n,,,:) 포함IllegalArgumentException"예약어는 구분자로 사용할 수 없습니다."4. 문자열 파싱 단계
IllegalArgumentException"숫자 형식이 올바르지 않습니다."IllegalArgumentException"음수는 입력할 수 없습니다."5. 계산 단계
IllegalArgumentException"계산할 숫자가 없습니다."3. 프로젝트 구조
4. 품질 요구사항