Conversation
There was a problem hiding this comment.
안녕하세요 리뷰어 흑곰입니다 !
프로그램 개발하신다고 고생 많으셨습니다.
현재 코드에서 기능마다 메서드와 객체를 나누고 코드의 응집도를 높이며 코드를 유지보수할 수 있도록 하면 좋을 것 같습니다.
MVC 패턴을 도입해서 각각의 역할을 지킬 수 있도록 하며, TDD 개발 방식을 도입하여 테스트를 작성하며 단위 테스트를 하며 프로그램의 안정성 및 유지보수성을 올리면 좋을 것 같습니다.
그리고 리드미와 요구사항에 적힌 내용을 지키지 않은 것 같습니다. 커밋 메세지와 개발 방식 등 요구사항을 지키며 개발을 진행하면 좋을 것 같습니다.
다른 PR 기록 이 여기 있습니다. 코멘트 순으로 여러 레퍼런스를 보고 커밋 기록을 보며 영감을 얻어가면 좋을 것 같습니다.
TDD 영상 도 확인해보세요 !
클린코드 체크리스트 레퍼런스 입니다 잘 확인해보세요 !
그리고 PR 코멘트나 리드미에 개발 의도 코드 작성의도 객체 분리 의도를 잘 작성해주고 문제 해결 과정 및 정의 과정도 잘 작성해주시면 좋을 것 같아요.
| if (input == null || input.isEmpty()) { | ||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
if (input == null || input.isEmpty()) {
return 0;
}현재 기능은 입력값을 검증하는 역할을 하는데 분리 할 수 있지 않을까요?
There was a problem hiding this comment.
입력값 검증 로직을 Validator 클래스로 분리하여
단일 책임 원칙을 지키도록 수정하겠습니다.
|
|
||
| String delimiter = "[,:]"; | ||
| String numberPart = input; | ||
|
|
There was a problem hiding this comment.
클래스도 나눠서 이 부분을 상수화나 기능을 잘 분리하면 좋을 것 같아요
There was a problem hiding this comment.
DEFAULT_DELIMITER를 상수로 선언하고 파싱 로직을
StringParser 클래스로 분리하겠습니다
| int idx = input.indexOf("\\n"); | ||
| if (idx < 0) { | ||
| throw new IllegalArgumentException("잘못된 커스텀 구분자 형식입니다."); | ||
| } |
There was a problem hiding this comment.
검증 기능의 역할을 하는 것 같아요 분리하는 방향으로 리팩토링 해보면 좋을 것 같아요.
There was a problem hiding this comment.
커스텀 구분자 형식 검증을 Validator 클래스로 분리하겠습니다.
| String numberPart = input; | ||
|
|
||
| if(input.startsWith("//")) { | ||
| int idx = input.indexOf("\\n"); |
There was a problem hiding this comment.
변수명 idx가 어떤 인덱스를 의미하는지 불명확했습니다.
newlineIndex로 변경하여 가독성을 높이겠습니다.
|
|
||
| int sum = 0; | ||
| for (String token : numberPart.split(delimiter)) { | ||
| int num; |
There was a problem hiding this comment.
num 변수를 parseNumber() 메서드로 분리하여
변수 선언 없이 바로 반환하도록 수정하겠습니다.
| for (String token : numberPart.split(delimiter)) { | ||
| int num; | ||
| try{ | ||
| num = Integer.parseInt(token.trim()); |
There was a problem hiding this comment.
구분자를 추출하고 공백을 제거한다는 기능이 두개가 들어가있는거 같습니다.
There was a problem hiding this comment.
공백 제거(trim)와 숫자 변환(parseInt) 두 가지 역할이 한 줄에 있었습니다.
parseNumber() 메서드로 분리하여 한 가지 역할만 담당하도록 수정하겠습니다.
| try{ | ||
| num = Integer.parseInt(token.trim()); | ||
| }catch (NumberFormatException e){ | ||
| throw new IllegalArgumentException("숫자가 아닌 값: " + token); |
There was a problem hiding this comment.
예외 메시지 형식을 요구사항에 맞게 확인하겠습니다.
"숫자가 아닌 값이 포함되어 있습니다: " 로 수정하겠습니다.
| throw new IllegalArgumentException("숫자가 아닌 값: " + token); | ||
| } | ||
| if (num < 0) { | ||
| throw new IllegalArgumentException("음수는 사용 불가: " + num); |
There was a problem hiding this comment.
음수 검증 로직을 Validator 클래스로 분리하겠습니다.
예외 메시지도 "음수는 입력할 수 없습니다: " 로 수정하겠습니다.
| System.out.println("덧셈할 문자열을 입력해 주세요."); | ||
| String input = Console.readLine(); | ||
|
|
||
| int result = calculate(input); |
There was a problem hiding this comment.
계산하는 기능에서 많은 역할을 책임하는 것 같습니다. 객체를 나누고 잘 분리를 하며 SRP를 지키면서 개발하면 좋을 것 같습니다.
MVC 패턴과 TDD를 도입해서 테스트를 작성해서 코드의 무결성을 검증하고 MVC 패턴을 통해서 모델, 입/출력, 컨트롤러에서 흐름을 각각 책임지도록 해서 코드의 응집도를 올리면 좋을 것 같습니다.
There was a problem hiding this comment.
말씀 감사합니다. MVC 패턴을 도입하여 각 클래스가 단일 책임만 가지도록 분리하였습니다.
- controller: CalculatorController → 전체 흐름 제어
- model: StringCalculator, StringParser, Validator → 계산, 파싱, 검증
- view: InputView, OutputView → 입출력 담당
…lculator 폴더로 이동 시켰습니다.
jyt6640
left a comment
jyt6640
left a comment
There was a problem hiding this comment.
전체적으로 기능 구현은 요구사항에 맞게 단순하고 명확하게 작성되어 있으며, 입력 → 계산 → 출력 흐름도 직관적으로 구성되어 있습니다. 메서드 단위 분리도 잘 되어 있어 가독성 측면에서는 큰 문제가 없습니다.
다만 구조적으로 보면 객체 간 협력보다는 static 메서드 호출 중심의 절차지향 흐름에 가까운 점이 아쉽습니다. CalculatorController, StringCalculator, StringParser, Validator, InputView, OutputView가 모두 static으로 연결되어 있어, 객체가 메시지를 주고받으며 책임을 나누는 구조라기보다 유틸리티 메서드를 조합하는 형태로 동작하고 있습니다.
MVC 관점에서 보면 Controller와 View는 분리되어 있지만, Model(도메인)이 거의 존재하지 않는 상태입니다. 현재 Model 역할을 하는 부분이 String, String[], int 같은 원시값에 머물러 있어, 애플리케이션의 핵심 규칙이 객체로 표현되지 않고 있습니다. 이는 MVC 구조에서 가장 중요한 도메인 계층이 비어 있는 것과 유사한 상태입니다.
특히 이 문제에서 중요한 도메인 개념은 계산식(Expression), 구분자(Delimiter), 숫자 목록(Numbers), 양수 숫자(PositiveNumber) 등인데, 이러한 개념들이 객체로 모델링되지 않고 단순 문자열/정수로 처리되고 있습니다. 그 결과 파싱 로직은 StringParser, 검증 로직은 Validator, 계산 로직은 StringCalculator에 분산되어 있으며, 각 객체가 자신의 규칙을 책임지지 못하고 있습니다.
예를 들어 음수 검증은 숫자 자체가 책임져야 자연스럽지만 현재는 외부 Validator에서 처리하고 있고, 커스텀 구분자 형식 역시 구분자 객체가 아닌 문자열 파싱 로직 내부에서 처리되고 있습니다. 이로 인해 규칙이 데이터와 함께 묶이지 않고 여러 클래스에 흩어지는 구조가 됩니다.
객체지향적으로 개선하려면 원시값을 그대로 사용하는 대신 도메인 객체로 추상화하는 것이 중요합니다. 예를 들어 입력 문자열은 Expression 객체로, 구분자 규칙은 Delimiter 객체로, 숫자 목록은 Numbers라는 일급 컬렉션으로, 음수 허용 여부는 PositiveNumber 같은 값 객체로 표현할 수 있습니다. 이렇게 되면 각 객체가 자신의 생성 시점에 유효성을 검증하고, 관련 로직을 내부에 캡슐화할 수 있어 책임이 명확해집니다.
또한 static 유틸리티 구조를 줄이고 객체 간 협력 구조로 전환하면, 향후 요구사항 변경(구분자 확장, 계산 방식 변경 등)에 대해서도 기존 코드를 수정하기보다 새로운 객체를 추가하는 방식으로 대응할 수 있어 SOLID 원칙(OCP, DIP)에도 더 부합하게 됩니다.
결론적으로 현재 구현은 기능 요구사항을 충족하는 데에는 충분하지만, 도메인 모델링이 부족하여 객체지향적인 설계의 장점을 충분히 살리지 못하고 있습니다. 메서드 분리 수준을 넘어서 도메인 개념을 객체로 끌어올리고, 각 객체가 자신의 규칙을 책임지는 구조로 리팩토링하면 더 좋은 설계가 될 것 같습니다.
| public class CalculatorController { | ||
| public void run(){ | ||
| String input = InputView.read(); | ||
| int result = StringCalculator.calculate(input); |
There was a problem hiding this comment.
static 호출로 인해서 계산 로직과 너무 강하게 결합되어 있다고 생각합니다.
DI 형태로 객체를 주입하면 테스트하기도 쉽고 확장성 개선도 가능하다고 봅니다
|
|
||
| public class StringCalculator { | ||
|
|
||
| public static int calculate(String input) { |
There was a problem hiding this comment.
static 메서드로 인해 상태/책임을 객체로 표현하지 못한 것 같습니다 -> OOP 위반이라고 생각합니다.
인스턴스 기반으로 코드를 리팩토링 해보면 좋을 것 같습니다.
| public static int calculate(String input) { | ||
| if (input == null || input.isEmpty()) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
이 기능은 이전에도 말한 것 같지만 입력 검증 책임이라고 생각합니다.
calculator에서 존재한다는 것은 계산 기능이라고 생각하는데 본 기능 같은 경우에는 입력 검증이라고 생각하기에 분리하면 좋을 것 같습니다.
즉 Input/Parser 쯔음에서 고민해보면 될 것 같습니다.
| if (input == null || input.isEmpty()) { | ||
| return 0; | ||
| } | ||
| String[] tokens = StringParser.parse(input); |
There was a problem hiding this comment.
파싱하는 것을 외부에서 하는 것은 맞다고 생각하지만 calculator에 존재하고 있기 때문에 DIP 위반이라고 생각합니다.
하나의 메서드 안에 많은 기능이 책임을 지고 있는 것 같습니다.
동일하게 잘 분리해보면 좋을 것 같습니다.
| return 0; | ||
| } | ||
| String[] tokens = StringParser.parse(input); | ||
| return sum(tokens); |
There was a problem hiding this comment.
sum에서도 내부에서 검증 + 변환을 같이 수행하고 있기 때문에 책임이 만다고 생각해보시면 좋을 것 같습니다.
합산 / 검증 / 변환을 분리해보면 좋을 것 같습니다.
| int newlineIndex = input.indexOf(CUSTOM_SUFFIX); | ||
| Validator.validateCustomDelimiterFormat(newlineIndex); | ||
|
|
||
| String custom = input.substring(2, newlineIndex); |
There was a problem hiding this comment.
substring 인덱스 2가 매직 넘버기 때문에 상수 처리를 해보면 좋을 것 같습니다.
|
|
||
| String custom = input.substring(2, newlineIndex); | ||
| String delimiter = DEFAULT_DELIMITER +"|" + Pattern.quote(custom); | ||
| String numberPart = input.substring(newlineIndex + 2); |
There was a problem hiding this comment.
이거도 동일하게 매직 넘버이기 때문에 유지보수가 어려울 것 같습니다.
잘 고민해보세요
| String delimiter = DEFAULT_DELIMITER +"|" + Pattern.quote(custom); | ||
| String numberPart = input.substring(newlineIndex + 2); | ||
|
|
||
| return numberPart.split(delimiter); |
There was a problem hiding this comment.
구분자가 문자열 그대로 들어가기 때문에 regex 문제가 있을 수 있습니다. Pattern.quote를 썻던 거로 기억하는데 사용하면 좋을 것 같습니다.
| package calculator.model; | ||
|
|
||
| public class Validator { | ||
| public static void validateCustomDelimiterFormat(int newlineIndex){ |
There was a problem hiding this comment.
이게 결국 parser 책임이기 때문에 책임 위치가 잘못 됬다고 생각하면 될 것 같습니다.
| if(number < 0){ | ||
| throw new IllegalArgumentException("음수는 입력할 수 없습니다: " + number); | ||
| } | ||
| } |
There was a problem hiding this comment.
숫자 검증을 외부에서 하고 있기 떄문에 객체지향이 아닌 것 같습니다. Number라는 클래스 즉 객체를 생성 시에 검증하도록 변경하면 좋을 것 같습니다.
2 participants
숙제