[문자열 덧셈 계산기] 주유나 미션 제출합니다.#228
Conversation
…ns to positive integers
| async run() { | ||
| Console.print("덧셈할 문자열을 입력해 주세요."); | ||
| try { | ||
| const input = await Console.readLineAsync(""); |
There was a problem hiding this comment.
이건 저도 마지막에 한 실수긴 하지만
const input = await Console.readLineAsync("덧셈할 문자열을 입력해 주세요.\n");
같이 하셨어도 깔끔했을것 같아요
There was a problem hiding this comment.
이건 저도 마지막에 한 실수긴 하지만
const input = await Console.readLineAsync("덧셈할 문자열을 입력해 주세요.\n");같이 하셨어도 깔끔했을것 같아요
헉.. 그때는 생각해보지 못했는데 훨씬 가독성도 좋아보이고 흐름이 좋네요 🙇🏻♀️ 리뷰 정말 감사합니다!
There was a problem hiding this comment.
나중에
덧셈할 문자열을 입력해 주세요.
같은 텍스트도 상수로 분리하는것도 고려해보시면 좋을것 같습니다!!
저 같은경우 오타를 많이내는 편이라 분리했을때 이점도 있더라고요
There was a problem hiding this comment.
나중에
덧셈할 문자열을 입력해 주세요.같은 텍스트도 상수로 분리하는것도 고려해보시면 좋을것 같습니다!!저 같은경우 오타를 많이내는 편이라 분리했을때 이점도 있더라고요
다음 미션에서 그부분도 고려해보겠습니다! 좋은 의견 감사드립니다 👩🏻💻
| const delim = input.slice(2, idx); | ||
| if (!delim || delim.length !== 1) { | ||
| throw new Error( | ||
| "[ERROR] 커스텀 구분자는 '//'와 '\\n' 사이의 문자 1개만 사용할 수 있습니다." |
There was a problem hiding this comment.
메세지 같은 경우, 언제나 유동적으로 바뀔 수 있는 부분이라고 생각해요!
상수화를 적용한다면 좀 더 관리하기 쉬워보일 것 같습니다:)
There was a problem hiding this comment.
메세지 같은 경우, 언제나 유동적으로 바뀔 수 있는 부분이
이 부분에 대해서 리뷰가 많아 다음 미션에는 꼭 적용해도록 해보겠습니다! 좋은 의견 감사합니다 😊
There was a problem hiding this comment.
특히 오타와 같은 휴먼애러에도 유용할것 같고요 !
에러 메세지 상수화 적용에 대해서 코드를 작성할때 생각하지 못했는데, 다른분들은 이 부분까지 미리 고려하신 것 같습니다!
말씀해주신 것처럼 저도 다음 미션때는 재사용성과 유지보수성을 위해 꼭 적용해보겠습니다 😁
|
|
||
| function escapeRegexChar(ch) { | ||
| return ch.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| } |
There was a problem hiding this comment.
이스케이프 처리를 별도 함수로 분리한 점이 좋네요!
저는 함수로 분리할 생각을 못하고 있었는데 추후 테스트를 작성할 때도 용이할 것 같아요👍
There was a problem hiding this comment.
감사합니다 😊
정규식 이스케이프 로직이 반복될 수 있다고 생각해서 별도 함수로 분리했습니다!
말씀하신것과 같이 테스트 코드 작성 시에도 따로 검증할 수 있어서 유지 보수성에 좋을거라 생각했습니다. 리뷰 감사드립니다!
There was a problem hiding this comment.
앗 저는 특수문자 처리까지는 생각하지 못했었는데 하나 배워갑니다..!!
음 이런 정규식도 따로 constant 상수로 처리하면 가독성이 조금 더 좋아질 것 같아서 리뷰 남깁니다!
There was a problem hiding this comment.
앗 저는 특수문자 처리까지는 생각하지 못했었는데 하나 배워갑니다..!!
음 이런 정규식도 따로 constant 상수로 처리하면 가독성이 조금 더 좋아질 것 같아서 리뷰 남깁니다!
피드백 감사합니다!
말씀주신 대로 상수로 분리하면 가독성을 더 높일 수 있을거같아요. 다음에 꼭 참고하겠습니다
| } | ||
| // 커스텀 구분자가 없을 때 | ||
| return { delims: DEFAULT_DELIMS, body: input }; | ||
| } |
There was a problem hiding this comment.
parseDelimiterAndBody에서 하는 기능들이 많아서 함수 분리를 하는건 어떻게 생각하시나요?
커스텀 구분자 추출, 검증, body 추출과 같이요!
There was a problem hiding this comment.
말씀해주신 대로 현재 하나의 함수가 여러 기능을 담당하고 있어서, 앞으로 함수의 역할 분리를 더욱 신경 쓰며 코드를 작성해야겠다는 생각이 듭니다! 좋은 피드백 감사합니다 😁
| declare module "@woowacourse/mission-utils" { | ||
| export const Console: { | ||
| readLineAsync(prompt?: string): Promise<string>; | ||
| print(message: string): void; | ||
| }; | ||
| } |
There was a problem hiding this comment.
library를 따로 분리해서 타입스크립트로 처리한 것 굉장히 인상적입니다!! 많이 배워갑니다 ㅎㅎ
There was a problem hiding this comment.
declare module 을 통해 타입스크립트화 하는 것은 처음 배웠습니다
해당 함수를 통해서 ide가 자동완성이 가능하게 하도록 하는것이 의의가 맞을까요 ?
d.ts로 전역 타입으로 선언함으로서 vscode가 자동완성되게 하신 의도 같아보이는데 맞는지 궁금하네요
There was a problem hiding this comment.
네 맞습니다! 😊
처음에 단순히 라이브러리를 사용하려고 헀는데
타입 선언이 없어 경고가 발생했었습니다.
이를 해결하기 위한 방법을 찾다가 declare module 을 통해 타입을 직접 정의하는 방식을 배웠습니다.
전역 타입(.d..ts) 으로 선언하여서 IDE 자동완성과 타입 추론이 가능하도록 설정했고, 경고 메세지가 사라졌습니다!
덕분에 타입 시스템 동작 원리에 대해서도 경험하였습니다ㅎㅎ
There was a problem hiding this comment.
library를 따로 분리해서 타입스크립트로 처리한 것 굉장히 인상적입니다!! 많이 배워갑니다 ㅎㅎ
리뷰 감사합니다 😁
에러는 아니였지만 경고가 발생해서 원인을 찾아보던 중
라이브러리에 타입 선언이 없다는 것을 알게 되었습니다!
declare module 을 통해 타입을 직접 정의하여 경고를 해결했고, 타입 시스템을 이해하는 데 도움이 된 즐거운 경험이였습니다ㅎㅎ
There was a problem hiding this comment.
저도 타입스크립트가 지원되지 않는 라이브러리일때 타입추론 하게 하는 방법을 배웠네요 .. 😁
There was a problem hiding this comment.
오 저도 타입 추론하는 법 하나 배워갑니다..! 신기하네요...:)
| return raw; | ||
| } | ||
|
|
||
| function toPositiveIntegers(tokens) { |
There was a problem hiding this comment.
함수명이 다소 불명확하다고 느낄 수도 있을 것 같아요..!
함수 안에 정규식 검사부터 숫자로 변환하는 기능까지 여러가지가 포함된 것 같은데
이것도 기능 분리를 해보면 좋을 것 같습니다!
There was a problem hiding this comment.
token이 의미하는것이 아마 number겠죠 ?
token이라고 하니까 구분자일것 같기도하고 문자일것 같기도하고 조금 모호한 부분이 있는것 같습니다
허나 전반적으로 token이 숫자라고 생각하고 보면 맥락이 이해가 가서 잘 작성된 코드라고 생각됩니다!
There was a problem hiding this comment.
함수명이 다소 불명확하다고 느낄 수도 있을 것 같아요..! 함수 안에 정규식 검사부터 숫자로 변환하는 기능까지 여러가지가 포함된 것 같은데 이것도 기능 분리를 해보면 좋을 것 같습니다!
함수 내부에서 다양한 기능을 처리하다 보니
함수명이 역할에 비해 불명확하게 느껴질 수 있겠네요 🤔
기능 분리 후에 그 역할에 맞는 명확한 함수명을 적용하는 연습을 해야겠습니다. 좋은 피드백 감사합니다 😊
There was a problem hiding this comment.
token이 의미하는것이 아마 number겠죠 ? token이라고 하니까 구분자일것 같기도하고 문자일것 같기도하고 조금 모호한 부분이 있는것 같습니다 허나 전반적으로 token이 숫자라고 생각하고 보면 맥락이 이해가 가서 잘 작성된 코드라고 생각됩니다!
문자열 형태의 숫자 조각 이라는 의미로 token 을 사용했는데 numberStrings 이나 numberToken 같은 좀 더 확실한 네이밍이 좋았을까요?ㅎㅎ 앞으로 직관적인 전달이 가능한 네이밍에 좀 더 신경써봐야겠습니다. 피드백 감사합니다! 😁
manNomi
left a comment
There was a problem hiding this comment.
전반적으로 코드가 깔끔하고 불필요한 파일이 없어서
너무 좋습니다 덕분에 많이 배웠습니다 좋은 코드 GOOD!!!
| async run() { | ||
| Console.print("덧셈할 문자열을 입력해 주세요."); | ||
| try { | ||
| const input = await Console.readLineAsync(""); |
There was a problem hiding this comment.
나중에
덧셈할 문자열을 입력해 주세요.
같은 텍스트도 상수로 분리하는것도 고려해보시면 좋을것 같습니다!!
저 같은경우 오타를 많이내는 편이라 분리했을때 이점도 있더라고요
|
|
||
| // 합산 | ||
| return numbers.reduce((acc, n) => acc + n, 0); | ||
| } |
There was a problem hiding this comment.
함수명을 수정해보는것은 어떨까요 ?
함수명이 add라서 어 덧셈을 해주는 함수구나
파라미터로 a,b와 같은 값이 들어오겠구나?를 예상했는데
input에 대한 파싱과 split 검증을 모두 진행하고 있었습니다
함수명을 수정하거나 기능을 쪼개도 좋을것 같긴합니다!!
There was a problem hiding this comment.
공통적으로 받은 피드백에서 함수 이름과 실제 역할이 명확히 드러내는 것이 얼마나 중요한지 느끼고 있습니다!
다음 코드에서는 보다 구체적인 함수명과 기능 분리를 명확히하여 코드를 개선해봐야겠습니다. 피드백 감사합니다 👍🏻
| return raw; | ||
| } | ||
|
|
||
| function toPositiveIntegers(tokens) { |
There was a problem hiding this comment.
token이 의미하는것이 아마 number겠죠 ?
token이라고 하니까 구분자일것 같기도하고 문자일것 같기도하고 조금 모호한 부분이 있는것 같습니다
허나 전반적으로 token이 숫자라고 생각하고 보면 맥락이 이해가 가서 잘 작성된 코드라고 생각됩니다!
| const delim = input.slice(2, idx); | ||
| if (!delim || delim.length !== 1) { | ||
| throw new Error( | ||
| "[ERROR] 커스텀 구분자는 '//'와 '\\n' 사이의 문자 1개만 사용할 수 있습니다." |
|
|
||
| if (idx === -1) { | ||
| throw new Error("[ERROR] 커스텀 구분자 뒤에 줄바꿈(\\n)이 필요합니다."); | ||
| } |
There was a problem hiding this comment.
해당 애러는 어떤 처리를 해주는 애러인가요??
//a\\n 123a123
과 같은 입력을 기대하는건가요 ?
There was a problem hiding this comment.
해당 에러는 커스텀 구분자 선언 뒤에 줄바꿈(\n)이 누락된 경우를 처리하기 위한 작성한 예외였습니다!
문제 요구사항에 커스텀 구분자를 "//"와 "\n" 사이의 문자로 정의했고, //;1;2;3 처럼 줄바꿈 없이 입력된 경우에 에러가 발생하도록 처리하였습니다!
| declare module "@woowacourse/mission-utils" { | ||
| export const Console: { | ||
| readLineAsync(prompt?: string): Promise<string>; | ||
| print(message: string): void; | ||
| }; | ||
| } |
There was a problem hiding this comment.
declare module 을 통해 타입스크립트화 하는 것은 처음 배웠습니다
해당 함수를 통해서 ide가 자동완성이 가능하게 하도록 하는것이 의의가 맞을까요 ?
d.ts로 전역 타입으로 선언함으로서 vscode가 자동완성되게 하신 의도 같아보이는데 맞는지 궁금하네요
|
1주차 미션 고생 많으셨습니다! 2주차도 파이팅해보아요!! |
| if (!delim || delim.length !== 1) { | ||
| throw new Error( | ||
| "[ERROR] 커스텀 구분자는 '//'와 '\\n' 사이의 문자 1개만 사용할 수 있습니다." |
There was a problem hiding this comment.
parseDelimiterAndBody 함수는 구분자 파싱과 검증 로직을 담고 있는데, 이 두가지 기능을 분리해보아도 좋을거 같아요!
There was a problem hiding this comment.
말씀해주신 것처럼 parseDelimiterAndBody가 파싱과 검증을 함께 처리하고 있어서, 역할 분리를 통해 구조를 더 명확히 하는게 좋을 것 같습니다. 좋은 피드백 감사합니다! 🙇🏻♀️
| if (raw.some((t) => t === "")) { | ||
| throw new Error("[ERROR] 구분자 사이에 숫자가 비어있습니다."); | ||
| } |
There was a problem hiding this comment.
의도하신건지는 모르겠습니다만 1,,2는 정상적으로 예외 처리가 되는데, 1,2,는 오류 메시지와 일치하지 않는 것 같아요!
There was a problem hiding this comment.
피드백 감사합니다! 👍🏻
확인해보니 말씀 주신 대로 1,2, 오류 메세지가 어색하네요ㅎㅎ.. 단일 메세지로 통일하여 간결하게 처리하거나,
시작/끝/연속 구분자 케이스를 구분해서 세분화 메세지를 던지는 방식을 생각해봤는데 어떨까요?
좋은 리뷰들 정말 감사합니다 🥹 |
피드백들 참고하여 더 나은 코드 짜보도록 하겠습니다! |
ohsung0722
left a comment
There was a problem hiding this comment.
1주차 고생 많으셨습니다!!
고민한 흔적이 많이 보이는 코드였던 것 같아요! 2주차도 화이팅입니다!!
|
|
||
| function escapeRegexChar(ch) { | ||
| return ch.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| } |
There was a problem hiding this comment.
앗 저는 특수문자 처리까지는 생각하지 못했었는데 하나 배워갑니다..!!
음 이런 정규식도 따로 constant 상수로 처리하면 가독성이 조금 더 좋아질 것 같아서 리뷰 남깁니다!
| const raw = body.split(re).map((t) => t.trim()); | ||
|
|
||
| // split으로 나눈 결과가 유효한지 검사 | ||
| if (raw.some((t) => t === "")) { | ||
| throw new Error("[ERROR] 구분자 사이에 숫자가 비어있습니다."); | ||
| } |
There was a problem hiding this comment.
변수명이 t, raw, re처럼 줄여 쓰여 있어서 처음 읽는 사람 입장에선 의미를 파악하기 어려울 수 있을 것 같아요!
주석으로 어떤 의미인지 명시해주거나, 네이밍을 조금 더 구체적으로 작성하면 가독성이 좋아질 것 같습니다!
There was a problem hiding this comment.
변수명이 t, raw, re처럼 줄여 쓰여 있어서 처음 읽는 사람 입장에선 의미를 파악하기 어려울 수 있을 것 같아요!
주석으로 어떤 의미인지 명시해주거나, 네이밍을 조금 더 구체적으로 작성하면 가독성이 좋아질 것 같습니다!
피드백 감사합니다. 저도 그부분 동의합니다!
좀 더 구체적인 네이밍을 사용해서 가독을 높여야겠네요 👩🏻💻
| declare module "@woowacourse/mission-utils" { | ||
| export const Console: { | ||
| readLineAsync(prompt?: string): Promise<string>; | ||
| print(message: string): void; | ||
| }; | ||
| } |
There was a problem hiding this comment.
오 저도 타입 추론하는 법 하나 배워갑니다..! 신기하네요...:)
MyungJiwoo
left a comment
There was a problem hiding this comment.
이미 좋은 리뷰가 많이 남겨져 있어서 최대한 중복되지 않는 부분으로 자세히 확인해보고 짧게나마 리뷰 남겨보았습니다!
특히 declare로 라이브러리 타입 추론하신 점이 인상적이었습니다. 😃
1주차 미션하시느라 고생하셨습니다!
| if (realNewlineIdx !== -1) { | ||
| idx = realNewlineIdx; |
There was a problem hiding this comment.
혹시 이 부분이 실제 개행으로도 커스텀 구분자를 사용할 수 있게끔 하기 위한 부분이 맞을까요..?
저는 아래와 같은 입력도 허용하기 위한 부분이라고 이해했는데, 잘 이해한게 맞는지 궁금합니다..!
//;
1;2;3
There was a problem hiding this comment.
만약 맞다면 사용자 입장에서 사용하기 더 좋은 것 같아요! ㅎㅎ
There was a problem hiding this comment.
혹시 이 부분이 실제 개행으로도 커스텀 구분자를 사용할 수 있게끔 하기 위한 부분이 맞을까요..? 저는 아래와 같은 입력도 허용하기 위한 부분이라고 이해했는데, 잘 이해한게 맞는지 궁금합니다..!
//; 1;2;3
네 맞습니다 👍🏻
이 부분은 실제 개행이 포함된 입력도 커스텀 구분자로 인식할 수 있도록 하기 위한 코드였습니다!
주신 예시처럼 //;\n1;2;3 형태로 실제 줄바꿈을 포함해 입력하더라도 정상적으로 처리되도록 의도하였습니다 😊
There was a problem hiding this comment.
오오 저도 타입스크립트의 declare로 라이브러리의 타입을 별도로 처리하는 방법에 대해 새롭게 배웠습니다! 감사합니다 😊
| let idx = -1; | ||
| let useLiteral = false; | ||
|
|
||
| if (realNewlineIdx !== -1) { | ||
| idx = realNewlineIdx; | ||
| } else if (literalNewlineIdx !== -1) { | ||
| idx = literalNewlineIdx; | ||
| useLiteral = true; | ||
| } | ||
|
|
||
| if (idx === -1) { | ||
| throw new Error("[ERROR] 커스텀 구분자 뒤에 줄바꿈(\\n)이 필요합니다."); | ||
| } |
There was a problem hiding this comment.
총 4곳에서 -1이 쓰이는데, 각각의 -1이 가진 의미나 맥락은 다르다고 생각합니다.
이때 -1을 상수로 만들어 그 맥락을 담아 사용하는 것은 어떨까요??
그렇다면 코드를 읽을 때 조금 더 쉽게 이해할 수 있을 것 같습니다!
There was a problem hiding this comment.
총 4곳에서
-1이 쓰이는데, 각각의-1이 가진 의미나 맥락은 다르다고 생각합니다. 이때-1을 상수로 만들어 그 맥락을 담아 사용하는 것은 어떨까요?? 그렇다면 코드를 읽을 때 조금 더 쉽게 이해할 수 있을 것 같습니다!
그렇네요! 상수로 분리하는 편이 훨씬 더 명확할 것 같습니다.
좋은 피드백 감사합니다 🙇🏻♀️
| } | ||
|
|
||
| export function add(input) { | ||
| const trimmed = (input ?? "").trim(); |
There was a problem hiding this comment.
(input ?? "")으로 입력 값을 방어하신 점이 좋은 것 같아요!
There was a problem hiding this comment.
예상치 못한 null이나 undefined 입력에서도 안전하게 동작하도록 방어 코드를 추가했습니다. 작은 부분이지만 알아봐주셔서 감사합니다 😊
🧩 문자열 덧셈 계산기 핵심 요약
,,:및 커스텀 구분자//<delim>\n지원결과 : 0출력[ERROR]처리App.js에서 Console 입출력,StringCalculator.js에서 계산 로직 분리.js확장자 누락 및readLineAsync인자 오류 수정