Skip to content

Conversation

@manNomi
Copy link

@manNomi manNomi commented Oct 20, 2025

추가조건사항

  • 커스텀 구분자를 여러개 입력받을 수 있다
  • 커스텀 구분자는 미리 선언되어야한다
  • 소수점은 허용하지 않는다 - 커스텀구분자로 (.) 이 오는 가능성을 배제하기 위해
  • 숫자는 커스텀구분자로 선언이 불가하다 - 사용자가 예상치 못한 방향으로 결과가 나올 수 있어서

🤔 고민했던 부분

MVC 패턴에서의 역할 분리

Model, View, Controller의 역할을 어떻게 명확하게 나눌지 고민했습니다.

특히, 입력값 검증(validation) 로직을 Model과 Controller 중 어디에 두는 것이 더 적합할지에 대해 깊이 생각했습니다. 현재는 Model이 자신의 데이터 상태를 검증하는 것이 객체지향적이라고 판단하여 Model에 구현했습니다. 이 부분에 대한 리뷰어님의 의견이 궁금합니다!

객체지향 원칙 적용

특히 Parser 클래스를 만들어 구분자 추출 및 숫자 파싱 로직을 분리했는데, 역할 분리가 잘 되었는지 검토 부탁드립니다.

테스트 코드의 품질

TDD를 진행하며 어떤 부분까지 테스트를 작성해야 할지 고민했습니다.

RandomMaker를 구현하여 다양한 무작위 입력값에 대해서도 계산기가 올바르게 동작하는지 테스트하고자 했습니다. 테스트의 경계값 설정이나 충분성에 대해 조언해주시면 감사하겠습니다.

📢 리뷰 요청 사항

클래스 설계: Parser, Number, Extraction 등 Model 내부의 클래스 설계가 객체지향 원칙에 부합하는지 피드백 부탁드립니다.

예외 처리: 현재의 예외 처리 방식(try-catch 및 에러 메시지 출력)이 요구사항에 맞게 잘 구현되었는지 확인 부탁드립니다.

코드 가독성: 변수명, 함수명이 코드를 읽는 사람이 이해하기 쉽게 작성되었는지 전반적인 가독성에 대한 의견을 듣고 싶습니다.

Copy link

@holdn2 holdn2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

많이 배워갑니다~!!

DIGIT: /\d/,
};

export const RegexUtils = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

간단한 함수들이라서 constant 안에 두신 것 같은데 이렇게 두는 것도 괜찮아 보이네요


const DEFAULT_DELIMITERS = [',', ':'];

const validate = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 모르는 것이 많아서 궁금한데 혹시 validate는 class로 만들지 않으신 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조금 정곡을 찔린 것 같습니다.

저는 **“행위는 객체로 두지 않는다”**는 원칙을 가지고 있었지만, 실제로는 Parser, Extractor처럼 행위적인 로직도 객체로 만들었던 부분이 있어서 아쉬움이 남는 1주차였습니다.

일관성을 지키려면 validate 역시 클래스가 되어야 맞겠지요.
하지만 validate를 클래스로 두지 않았던 이유는 명확했습니다.
검증에는 두 가지 종류가 있다고 생각했기 때문입니다.
1. 도메인 검증 – 입력이 의도에 맞는지 확인하는 것
2. 문법적 검증 – 입력이 형식적으로 올바른지 확인하는 것

이 두 검증이 클래스 안팎에서 모두 사용될 수 있다면,
validate를 클래스로 두었을 때 오히려 책임이 모호해질 수 있다고 판단했습니다.

앞으로는 문법적 검증은 함수로, 도메인 검증은 클래스 내부 메서드로 분리하여 작성하려 합니다.
좋은 피드백을 통해 제 원칙을 다시 점검할 수 있었던 의미 있는 경험이었습니다.


// 구분자 추출 {raw:원본,escaped:정규식 이스케이프}
const { raw, escaped } = this.extraction.extractCustomDelimiters(input);
validate.validateDelimiters(raw);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외 처리를 따로 빼서 확실히 분리하신 것이 좋아보입니다!

Copy link

@glory-yun glory-yun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1주차 고생 많으셨습니다!! 도전 사항이랑 테스트 코드를 작성하신 것 보고 정말 열심히 하셨다 생각했습니다 🔥🔥 너무 잘하셔서 짧은 코멘트 밖에 할 수 없었네요 😅 2주차도 화이팅입니다!!

-[X] 프로그래밍의 시작지점은 App.js의 run() 인가? -[X] package.json 파일을 변경하지 않았는가? -[X] 프로그램 종료시 process.exit()를 호출하지 않았는가? -[X] 자바스크립트 코드 컨벤션에 맞게 작성했는가? -[X] @woowacourse/mission-utils에서 제공하는 Console API를 사용해서 구현했는가?

## 3. 도전 사항

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도전사항 세세하게 작성하신거 진짜 대단하십니다...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

문자열이나 상수를 constant로 묶은 것은 많이 봤는데, regex 관련된 것들만 따로 모아둔 것이 신선한거 같아요.
다른 분들 코드를 봤을 때는 util과 관련된 것 끼리 묶었는데, util 말고 regex와 관련된 것을 묶으신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regex를 따로 모아두고 util 대신 constant로 쓴 이유를 여쭤보신것 맞나요 ?
regex 또한 변하지않는 상수로 판이되어서 util 보다는 상수로 관리했고 !!
간단한 함수들을 util에 또 분리한다면 가독성을 해칠것 같아 같이 두었는데

고민이 되는게 지금은
regex 함수가 초점이면 utils regex 보관이 초점이면 constant에 보관할것 같은데 생각해보게 하는 좋은 글이네요 감사합니다

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외처리 함수가 많지 않기 때문에, 객체로 묶지 않고 각각 export 하는것도 괜찮아 보입니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 타당한 말씀이네요
각각 export 하면서 선언적으로 사용하면 더더욱 좋다는 생각이 드는것 같습니다

.filter((value) => value !== '')
.map((value) => Number(value));
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메서드 체이닝 때문에 코드가 길어지면 가독성이 떨어질 수 있을 것 같아요!
(공유받은 클린코드 공유 자료입니다! 시간 나실때 한번 읽어보세요!!)
https://github.com/woowacourse/woowacourse-docs/blob/main/cleancode/pr_checklist.md

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메소드 체이닝 때문에 코드가 읽히기 ㅅ쉽지 않다는것을 저도 느끼고 있습니다..

과도한 분리 어떨때는 너무 심한 체이닝 저 자신을 돌아보게 되네요 좋은 글 공유해주셔서 감사합니다 !

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 분들 코드를 참고해보니까 Extraction, Number, Parser를 Calculator class에서 다루시는 분들이 많더라고요! 개인적으로 Calculator에서 관리하는것도 괜찮아 보입니다!!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀주신 내용 전적으로 공감합니다
저 또한 너무 많은 분리가 오히려 가독성을 해치는것 같기도 하고
class 가 가지는 함수가 하나 뿐이라서 합치는게 오히려 올바르다고 생각이 드네요
행위 자체를 객체로 설정한것도 후회가 되고요 말씀 감사합니다!!

Copy link

@ohsung0722 ohsung0722 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적으로 정말 배울 점이 많았던 코드였습니다!

특히 저는 그동안 JavaScript에 MVC 구조를 직접 적용해본 적이 없었는데,
만욱님 코드를 보면서 JavaScript에서도 MVC 아키텍처를 명확하게 적용할 수 있구나라는 생각을 하게 된 것 같아요!
다음 과제부터는 저도 다양한 아키텍처 구조를 찾아보고, 직접 적용해보며 비교해볼 계획입니다. :)

리뷰 포인트 중 Model의 데이터 검증 책임에 대한 부분도 깊이 고민해봤습니다.
저 역시 Model이 자신의 데이터 상태를 스스로 검증하는 방식이 객체지향적으로 적합하다고 생각합니다.
다만 검증의 종류를 세분화했을 때, 일부는 Controller에 두는 것도 충분히 타당하다고 봅니다.

좀 보충해보자면, 저는 데이터 검증을 크게 두 가지로 나누어 생각합니다.
• 입력 형식 검증(Input Validation) : 사용자의 요청이 문법적으로 올바른지, 비어 있지 않은지 등을 확인하는 단계
• 도메인 규칙 검증(Domain Validation) : Model 내부 객체의 상태가 비즈니스 규칙을 만족하는지를 확인하는 단계

이 관점에서 봤을 때, 현재 만욱님처럼 Controller에서 별도의 validate 모듈을 불러와 입력 형식을 검증하고,
Model에서는 비즈니스 로직과 관련된 검증만 수행하는 구조는 책임이 명확히 분리되어 있어 매우 인상 깊었습니다.

정말 많이 배우고 갑니다. 좋은 코드와 리뷰 감사드립니다!

Comment on lines +1 to +11
export const ERROR_MESSAGE = {
NOT_MINUS: '[ERROR] 음수가 포함되어 있습니다.',
NOT_EMPTY: '[ERROR] 공백이 포함되어 있습니다.',
NOT_NUMBER: '[ERROR] 숫자가 아닌것이 포함되어 있습니다.',
DELIMITER_CONTINUOUS: '[ERROR] 연속된 구분자가 존재합니다.',
DELIMITER_EMPTY: '[ERROR] 구분자가 공백입니다.',
DELIMITER_HAS_WHITESPACE: '[ERROR] 구분자에 공백이 포함되어 있습니다.',
DELIMITER_HAS_NUMBER: '[ERROR] 구분자에 숫자가 포함되어 있습니다.',
DELIMITER_NOT_SINGLE_CHAR: '[ERROR] 구분자는 한 글자여야 합니다.',
NOT_INTEGER: '[ERROR] 소수가 포함되어 있습니다.',
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런식으로 객체로 묶어서 constants를 표현하는 것도 좋은 것 같아요!
좋은 방식 배워갑니다:)

@@ -0,0 +1,11 @@
export default class Number {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰하다가 validate.js에서 js 내장 객체인 Number()를 사용하시는데
이 부분 클래스 네이밍이 기존 js의 내장 Number랑 겹칠 수도 있을 것 같아서요!
좀 더 클래스명을 구체적으로 작성하는 것도 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 사용하다 보니 계속 Number가 겹치더라고요
내장코드랑 동일한 함수 , 클래스명을 짓다니 ....
덕분에 다음에는 실수를 줄일 수 있겠네요 감사합니다!

let result = text;

delimiters.forEach((delimiter) => {
const definition = RegexUtils.createCusomPattern(delimiter);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기 부분 진짜 사소하긴 한데 createCustomPattern() 함수 오타 있는 것 같아서 리뷰 남깁니다..!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자바스크립트라 오타에 치명적일텐데!!!
복사붙여넣기를 잘해서 아직 살아있나 봅니다 꼼꼼히 봐주셔 감사합니다 !!

Comment on lines +7 to +25
validateNumbers(numbers) {
numbers.forEach((number) => {
// 공백 체크
if (typeof number === 'string' && number.trim() === '') {
throw new Error(ERROR_MESSAGE.NOT_EMPTY);
}
// 숫자 여부 체크
if (Number.isNaN(Number(number))) {
throw new Error(ERROR_MESSAGE.NOT_NUMBER);
}
// 음수 체크
if (number < 0) {
throw new Error(ERROR_MESSAGE.NOT_MINUS);
}
// 소수 체크
if (!Number.isInteger(Number(number))) {
throw new Error(ERROR_MESSAGE.NOT_INTEGER);
}
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 Number(number)를 여러 번 호출하는 것 같습니다!
number라는 하나의 변수에 대해서 Number()로 변환하는 것이니 상단에

const num = Number(number)

로 변환은 한번만 수행하고, 비교 연산은 num만 받아와서 하는 방향도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼하게 봐주셔서 정말감사합니다
forEach 구문자체에서도 Number를 매번 호출한느데 심지어 체크할때마다 또 호출하네요...

Copy link

@whitecity01 whitecity01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MVC 패턴을 적용하기 위해 구조를 고민하신 흔적이 잘 드러납니다.
클래스별 역할이 명확하게 구분되어 있고, 객체지향적인 설계도 잘 이뤄진 코드라고 생각합니다.

다만, Controller가 입력, 파싱, 검증, 연산 등 다양한 역할을 동시에 수행하고 있어
역할이 다소 집중된 인상이 있습니다.
유효성 검사는 각 Model(Parser, Number) 내부에서 수행하고, 파싱과 연산과 같은 실질적인 비즈니스 로직은 Service 계층으로 분리하는 것도 좋은 방법이라 생각됩니다.

예외 처리 로직과 테스트 코드 부분은 특히 인상 깊었습니다.
처음에는 다소 세세하게 느껴졌지만, 사용자 경험을 고려하면 프론트엔드에서의 세밀한 에러 처리가 오히려 큰 장점이 된다고 생각합니다.

나머지 세부적인 피드백은 코멘트로 남겨뒀습니다.
덕분에 많이 배워갑니다~ 수고하셨습니다!

Comment on lines +16 to +17
const input =
await inputView.readLineMessage('덧셈할 문자열을 입력해 주세요.');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러메세지에 대해 상수로 분리하신 것을 보았습니다.
통일성을 위해 이 입력 문자열도 상수화 하는 것이 좋아보이는데 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지적 해주셔서 알게되었ㄴ네요...
상수화 한다고 해놓고 까먹었네요 실제 프로젝트다면 꼼꼼하게 봐주신 작성자 분덕분에
실수 하나를 줄일 수 있었겠네요 감사합니다!!

Comment on lines +30 to +33
const textToValidate = inputText.replace(
REGEX_PATTERNS.CUSTOM_DELIMITER_DEFINITION,
'',
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 저는 구분자 하나하나 비교를 수행했는데 모두 공백으로 바꾸는 방법이 있었네요!
좋은 방법인 것 같습니다

Comment on lines +1 to +11
export default class Number {
#numbers;

constructor(numbers) {
this.#numbers = numbers;
}

getAddedNumbers() {
return this.#numbers.reduce((acc, cur) => acc + cur);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number는 자바스크립트의 내장 객체 이름이라 혼동을 줄 수 있을 것 같습니다.

클래스명을 변경하거나, 단순한 기능이라면 별도 함수로 분리하는 것도 좋을 것 같습니다!

Copy link

@janghw0126 janghw0126 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1주차 미션 정말 고생 많으셨습니다!🥹
전체적으로 MVC 패턴의 구조가 잘 드러나서, 코드를 보며 자연스럽게 흐름을 이해할 수 있었어요. 덕분에 어떻게 MVC 패턴을 짜야하는지 배우게 되었습니다. 코드 구조도 정말 깔끔하고, 기능별로 모듈을 잘 분리한 부분이 인상 깊었습니다. 정말 많이 배워갑니다! 감사합니다😊

},

createContinuousPattern(delimiter) {
return new RegExp(`${delimiter}{2,}`);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

문자열의 패턴을 관리하는 RegExp객체가 있다는 것을 새롭게 알게 되었습니다.
반복적으로 사용하는 정규식을 한 곳에 모아두니 코드가 훨씬 깔끔해지네요! 😊

export default class Extraction {
extractCustomDelimiters(text) {
const delimiters = Array.from(
text.matchAll(REGEX_PATTERNS.CUSTOM_DELIMITER_EXTRACT),
Copy link

@janghw0126 janghw0126 Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matchAll과 Array.from을 함께 사용하는 이유를 명확히 알고 싶습니다!
단순히 match()로는 안 되는 건가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 처음에는 match를 사용했었는데
커스텀 구분자를 여러글자 받기위해서 matchAll 을 사용했습니다!!
한글자를 입력받게 하신분들이 많더라고요 !!
그래서 아마 차이가 나는것 같습니다

Comment on lines +1 to +10
export const ERROR_MESSAGE = {
NOT_MINUS: '[ERROR] 음수가 포함되어 있습니다.',
NOT_EMPTY: '[ERROR] 공백이 포함되어 있습니다.',
NOT_NUMBER: '[ERROR] 숫자가 아닌것이 포함되어 있습니다.',
DELIMITER_CONTINUOUS: '[ERROR] 연속된 구분자가 존재합니다.',
DELIMITER_EMPTY: '[ERROR] 구분자가 공백입니다.',
DELIMITER_HAS_WHITESPACE: '[ERROR] 구분자에 공백이 포함되어 있습니다.',
DELIMITER_HAS_NUMBER: '[ERROR] 구분자에 숫자가 포함되어 있습니다.',
DELIMITER_NOT_SINGLE_CHAR: '[ERROR] 구분자는 한 글자여야 합니다.',
NOT_INTEGER: '[ERROR] 소수가 포함되어 있습니다.',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT_MINUS나 NOT_NUMBER에 어떤 게 음수가 아니고 어떤 게 숫자가 아닌지 불분명한 것 같아서
처음 보는 사람이 코드를 읽으면 정확히 눈에 안 들어올 수도 있을 것 같습니다!

에러 메시지는 어차피 상수이니 조금 더 구체적으로 명시가 되면 가독성이 더 좋아질 것 같아요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOT_MINUS인데 막상 글자와 하는역할은 포함되어 있다고 느껴지네요 말씀감사합니다

Comment on lines +41 to +43
if (continuousPattern.test(textToValidate)) {
throw new Error(ERROR_MESSAGE.DELIMITER_CONTINUOUS);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const validate = {
부터 이부분까지의 depth가 다소 깊은 것 같습니다!

depth를 줄이기 위해서 forEach부분도 따로 함수로 뺄 수 있을 것 같고
검증 로직도 아예 함수로 빼서 if문을 감출 수도 있을 것 같습니다 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀 감사합니다 이전주에 짠 코드임에도 잘 읽히지 않는것을 보니
가독성이 많이 떨어지게 작성한것 같습니다
depth를 줄이기 위해 방법을 추천해주셔서 감사합니다

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러 코드를 하나 하나 자세히 적어주셔서 에러가 났을 때 어떤 에러인지 한 번에 알 수 있을 것 같아요!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적으로 함수와 class를 굉장히 잘 활용하신다고 느꼈습니다. 저도 다음엔 기능 별로 나눠보도록 해봐야 겠어요. 많이 배우고 갑니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이해하고 쓰는거랑 알고 쓰는것의 차이를 많이 느끼고 있습니다..
그저 method만 알고 방법만 알고 적고 있는 기분이라 다른 팀원들도
class를 적극적으로 활용해서 같이 공부해보면 좋을것 같습니다!!

Copy link

@YounaJ00 YounaJ00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기능 단위로 역할을 명확히 나눈 구조가 인상적입니다 👍🏻
특히 랜덤 테스트 파일 작성 방식에서 많이 배웠습니다!
좋은 코드 잘 봤습니다. 다음 미션도 화이팅입니다! 😊

Comment on lines +8 to +10
getAddedNumbers() {
return this.#numbers.reduce((acc, cur) => acc + cur);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빈 문자열 입력 등으로 숫자 배열이 비어질 수 있는데
현재 reduce 함수에는 초기값이 없어서 예외가 발생할 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 그러네요!! 초기값 지정을 했어야 했는데 감사합니다 !

Comment on lines +1 to +3
export const REGEX_PATTERNS = {
CUSTOM_DELIMITER_EXTRACT: /\/\/(.*?)\\n/g,
CUSTOM_DELIMITER_DEFINITION: /\/\/.+?\\n/g,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 코드라면 \ + n 를 찾아서 \n 처리에 문제가 생길 수도 있지않을까 궁금합니다!

@JaeHye0k
Copy link

안녕하세요 1주차 고생 많으셨습니다!

기술적인 내용은 아니지만 커밋 메시지를 "~하다"형식으로 작성하셨는데, 개조식으로 작성하면 좀 더 눈에 잘 들어올 것 같습니다!

예: "기능요구사항을 정의하다" -> "기능요구사항 정의"

아마 Angular JS 컨벤션에 동사로 표시되어있어서 이렇게 작성하신걸로 유추됩니다.
하지만 한글엔 "은/는/이/가/을" 조사들과, 동사로 변형했을 때 오히려 글자수가 많아지는 특징 때문에 오히려 위와같은 개조식이 더 깔끔해 보일 수 있을 것 같아요!

WHITESPACE: /\s/,
DIGIT: /\d/,
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정규식을 한번에 묶어둘 생각은 못했는데, 정규식이 코드에서 반복되는 것을 막아 좋은 것 같네요!

Copy link

@MyungJiwoo MyungJiwoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 입력값의 유효성 검사를 어떤 흐름에서 하는지 고민을 많이 했었는데, MVC 패턴에서는 어떤 부분에 적합할지 고민이되네요..! (피드백 요청해주셨는데 답변이 되지 못해 죄송합니다 🥲..)

코드와 남겨진 댓글들을 보며 코드 한 줄에 의도를 담는다는게 이런거구나 느껴졌습니다.
명목상 코드 리뷰지만 여러모로 제가 더 많이 배워가는 리뷰였습니다. (특히 코드의 구조나 객체지향, TDD 측면에서 많이 배울 수 있었어요!)

짧게나마 제 생각 남겨두었습니다. ㅎㅎ 1주차 미션 고생하셨습니다!


## 3. 도전 사항

-[X] TDD 설계원칙 적용 -[X] 테스트 코드 랜덤 문자열 생성기 구현 -[X] MVC 패턴 적용하기 -[X] 객체지향 원칙 준수하기 -[X] AI 사용 없이 구현하기

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI 사용 없이 구현에 성공하셨군요..!! 대단하십니다 ..👍🏻

@@ -0,0 +1,11 @@
export const ERROR_MESSAGE = {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

런타임에서 변하지 않을 에러 메시지라면 Object.freeze를 사용하는 것도 좋다고 생각합니다.
혹시 일부러 사용하시지 않은거라면, 어떤 이유였을지 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

평소에 개발을 할때에는 object.freez가 필요가 없었어서 자주 사용을 안하다보니 안좋은 습관이 들은것 같습니다..
말씀하신것처럼 반영하는게 더 좋을것이라고 판단이 되네요

이번기회에 object.freez에 대해서 조금더 알게되었는데 얕은불변성만 보장한다는 사실을 처음 알게되었습니다 감사합니다!!

Comment on lines +23 to +25
const textWithoutDefinitions =
this.parser.removeCustomDelimiterDefinitions(input, raw);
validate.validateContinuousDelimiters(textWithoutDefinitions, escaped);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수명이 자세해서 특별한 주석 없이도 가독성이 좋은 것 같아요! 👀

import { ERROR_MESSAGE } from '../../constant/error.js';
import { REGEX_PATTERNS, RegexUtils } from '../../constant/regex.js';

const DEFAULT_DELIMITERS = [',', ':'];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parser.js의 3번째 줄도 같은 코드가 있는 것 같은데 constant 파일로 옮겨 재사용해도 괜찮을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

끝나고 코드리뷰를 받다보니 이런 실수들을 잡아주셔서 너무 좋네요 꼼꼼히 봐주셔 감사합니다

customDelimiters.forEach((delimiter) => {
// 빈 값 체크
if (!delimiter) {
throw new Error(ERROR_MESSAGE.DELIMITER_EMPTY);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외 처리에 대해 알아보다가 커스텀 에러 객체에 대해 알게 되었습니다.
저도 문자열 계산기에서 일반 Error 객체를 사용했는데, 커스텀 에러 객체를 사용하면 어떤 에러가 발생했는지 명확히 알 수 있어서 좋을 것 같더라구요.
단순하게 궁금해서 여쭤보고 싶은데 커스텀 에러 객체 사용에 대해서는 어떻게 생각하시는지 여쭤보고 싶습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error 객체를 extends해서 쓰는것 너무 좋을 것 같은데요 ???
다만 코드와 파일이 늘어나다보니 확실하게 커스텀 애러 객체가 하는일을 명확히 해야겠군요
사용 좋은것 같습니다

throw new Error(ERROR_MESSAGE.DELIMITER_HAS_NUMBER);
}
// 구분자가 한글자인지 체크
if (delimiter.length !== 1) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아주 사소한 부분이지만요.. 1을 상수화하여 매직넘버를 최소화 하는 것은 어떠신가요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 좋습니다!
허나 과도한 상수화가 오히려 가독성을 망칠 수 있다고 생각해서 고민이긴합니다
if (delimiter.length !== DELIMETER_MIN_LENGTH)
처럼 작성한다면 더 좋을것 같네요 수정에도 용이할것 같고요

Copy link

@mindaaaa mindaaaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 잘 보고 갑니다!

추가로 궁금한 점이 있는데, 커밋 메시지를 작성하시는 방법이 조금 특이한 것 같습니다.
feat: 구분자가 한글자만 허용할것인지 수정하다

이 메시지가 제가 생각하기에는 조사가 없고,
명령형 어미를 붙여 조금 부자연스럽게 느껴지는 것 같습니다.

일반적으로, feta: 구분자 허용 조건 수정 (한 글자 제한 여부 조정)
이런식의 요약형 설명을 주로 사용하는 것 같은데,
이런 식의 커밋 컨벤션을 사용하시는 이유가 궁금합니다!

const getRandomNumber = (start = 1, end = 9) =>
Random.pickNumberInRange(start, end);

// 커스텀 구분자 생성기

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 자동 테스트 도구인가요?
이런 방식은 생각도 못했는데, 기발한 방식으로 테스트하셨군요!
테스트도구만 믿지 않고 다양한 테스트케이스를 직접 만드신게 멋있습니다.

const customRegexs = [];
for (let i = 0; i < customRegexCount; i++) {
let charCode;
do {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적인 취향이지만, 저는 do~while 문을 별로 선호하지 않습니다.
조건을 나중에 보게되다보니 한 번 더 생각해야하기 때문인데요.

코드 한 토막만 보면 문제되지 않지만,
아시다시피 봐야할 코드가 정말 많기 때문에

let charCode = Random.pickNumberInRange(33, 126);
while (charCode >= 48 && charCode <= 57) {
  charCode = Random.pickNumberInRange(33, 126);
}

이런 식으로 사용하시면 더 가독성 있는 코드가 될 것 같다는 생각이 들기도합니다만..

이 글에서 보시면, 확실히 의견이 갈리다보니 이쪽이 더 편하시면 또 상관없겠네요! 한 번 고려해보면 좋을 것 같아 의견 남겨봤습니다 :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 오히려 처음에는 작성자 분이 작성해주신 코드를 따라갔었는데 추후 리팩토링 하면서
do while 로 수정했었거든요 저또한 글의 내용처럼 if문의 depth를 줄일 수 있는것 처럼 사용했었고요
개발하다보면 while문을 쓸일이 크게 없다보니 이런 고민을 못했던것 같은데 덕분에 생각할 수 있었네요
너무 좋은 글 첨언해주셔 감사합니다 !

};

// 랜덤 인풋 생성기
export const getRandomInput = () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아무래도 유틸함수라 그런 걸 수 있지만,
함수가 전반적으로 좀 긴 것 같습니다!

로직이 한 눈에 안 들어와서
하위함수로 분리하거나, 혹은 jsDoc 방식으로 설명을 좀 달아주면 좋을 것 같습니다.

getRandomInput의 경우,

  1. 에러 케이스 로드
  2. 랜덤 데이터 생성
  3. 결과 포맷 조립

세 가지 책임을 갖고 있는 것 같아 그 기준으로 나누시거나, 혹은 이런 식으로 jsDoc 명시를 해주시면 더 좋을 것 같습니다.

/**
 * 랜덤 입력 문자열을 생성하거나, 저장된 에러 케이스를 불러옵니다.
 * 
 * 내부 역할:
 * 1. 에러 케이스 존재 시 → 파일에서 JSON 로드
 * 2. 없을 경우 → 랜덤 구분자와 숫자 문자열 생성
 * 3. 생성된 입력 문자열과 기대 결과 반환
 * 
 * @returns {[string, number, number[], string[]]} 
 * input 문자열, 결과 합계, 파싱된 숫자 배열, 사용된 커스텀 구분자 배열
 */

이 글이 사용법과 예시, 장점을 잘 설명 한 것 같네요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀 주신것처럼 해당 함수는 유틸함수이며 테스트코드에서만 사용되기에
가독성이 떨어지더라도 구현에 초점을 맞추었다보니 로직이 제가 보기에도 이해가 힘든것 같네요
jsDoc으로 명시하는 방법을 알지 못했었는데 덕분에 깊이있게 배운것 같습니다
특히 코드가 읽기힘들텐데 읽어주셔서 감사하네요 ..

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 커밋 메시지
    좋은 지적 감사합니다!

저는 커밋 메시지를 쓸 때 **“현재 작업 중인 행동을 그대로 기록한다”**는 느낌으로 작성하는 편입니다.
그래서 ~하다, ~수정하다 같은 형태를 사용해서 자기 기록용 메모처럼 남기는 스타일이었습니다.

말씀하신 것처럼
feat: 구분자 허용 조건 수정 (한 글자 제한 여부 조정)
처럼 요약형 명령문 스타일이 더 일반적이고 깔끔하다는 것 같다는 생각이 드는것 같습니다.

또는
feat: 구분자 허용 조건 수정
description : 한 글자 제한 여부 조정
위 방식이 더좋겠다는 생각이드네요
커밋메시지에 고민이 많았는데 덕분에 좋은 정보를 얻어가는 것 같습니다
말씀감사합니다

@jhw030306
Copy link

제가 캐치한 부분은 다 다른 분들이 피드백을 주셔서,,!!
mvc 패턴도 너무 잘 쓰신 것 같고 일관성 있게 에러메시지를 처리하시는 것도 가독성이 더 높아보입니다!
특히 랜덤 테스트 방식 사용하신 것 보고 진짜 많은걸 배우고 가는 것 같아요!
1주차도 수고하셨습니다!!

@soyeong0115
Copy link

저는 기능별로 각각 파일을 분리하여 구현했는데, mvc 패턴으로 파일을 구조적으로 분리하신 점이 확실히 좋은 것 같네요!
리뷰 남기러 왔는데 @manNomi 님 코드와 다른 분들이 남겨주신 코멘트 보고 많이 배우고 갑니다! ㅎㅎ
저도 2주차에 에러메세지 분리, mvc 패턴 사용 등 직접 적용해 보겠습니다!

Copy link

@seahkeem seahkeem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스 분리가 잘 되어 있어서 가독성이 좋습니다!

Comment on lines +53 to +61
// 에러 발생 시 즉시 저장하고 테스트 중단
saveErrorCase(
input,
output,
parseMumber,
extracionRegex,
i,
err.message,
);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오류 케이스 발생 시 저장하는 부분이 인상깊어요. 테스트는 단순히 견고성 검증 정도로만 생각했는데 오류가 발생하는 경계값과 조합 같은 것도 확인할 수 있었네요. 나중에 오류 재현/수정에도 편리할 것 같습니당!

@manNomi manNomi changed the title [문자열 덧셈 계산기] 한만욱 과제 제출합니다. [문자열 덧셈 계산기] 한만욱 미션 제출합니다. Oct 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.