[문자열 덧셈 계산기] 이혁 미션 제출합니다. #234
Conversation
janghw0126
left a comment
janghw0126
left a comment
There was a problem hiding this comment.
전체적으로 기능이 잘 분리되어 있고, 각 함수 이름만 봐도 역할이 명확해서 읽기 편했습니다.
입력 → 파싱 → 계산의 흐름이 한눈에 보여서 구조적으로 깔끔하다고 느꼈습니다. 수고하셨습니다!
다음 주차도 파이팅입니다.
| @@ -1,5 +1,20 @@ | |||
| import { Console } from "@woowacourse/mission-utils"; | |||
| import { | |||
There was a problem hiding this comment.
모듈을 나누어 import하는 구조로 코드를 작성할 수 있군요!
저는 기능 구현에 집중하느라 가독성 있게 구조를 나누는 방법까지는 생각하지 못했는데, 이 코드를 보고 배웠습니다 :)
| let separators = [":", ","]; | ||
| if (hasCustomSeparator(input)) { | ||
| const customSeparators = input.match(pattern)[1]; | ||
| isLegelSeparator(customSeparators); |
There was a problem hiding this comment.
isLegalSeparator()로 수정하면 더 명확할 것 같습니다 :)
|
|
||
| for (const char of expressions) { | ||
| if (separators.includes(char)) { | ||
| number = Number(stack); |
There was a problem hiding this comment.
let number = Number(stack);처럼 명시적으로 선언해주면 안전할 것 같습니다 !
또는 함수 내부에서 const currentNumber = Number(stack);처럼 이름을 명확히 해주면 더 좋을 것 같아요
혹시 선언을 안한 이유가 따로 있으실까요?
There was a problem hiding this comment.
엇 저부분 무슨 생각을 하고 작성했는지 기억이 안나네요 다음부터는 조금더 깊게 생각하고 작성해봐야 할거 같습니다!
| let stack = ""; | ||
| let result = 0; | ||
|
|
||
| for (const char of expressions) { |
There was a problem hiding this comment.
문자열을 한 글자씩 순회하면서 직접 숫자를 만들어가는 구조가 흥미로웠어요! 평소엔 split()으로 나누는 방식만 생각했는데, 이런 식으로 스택처럼 직접 쌓았다가 처리하는 방법도 가능하다는 걸 배웠습니다.
salmonco
left a comment
salmonco
left a comment
There was a problem hiding this comment.
코드가 간결해서 잘 읽었습니다. 구분자 패턴을 제외한 문자열을 뭐라고 불러야 할지 고민이었는데 expression이라고도 표현할 수 있구나 싶어서 좋은 방법인 것 같았어요.
function.js이란 한 파일에 함수가 다 담겨 있는데, 담엔 좀 더 쪼갤 수 있을 것 같단 생각이 들었습니다. 2주차도 화이팅입니다!
| @@ -0,0 +1,61 @@ | |||
| const pattern = /\/\/(.*)\\n/; | |||
There was a problem hiding this comment.
[제안]
상수로 보여서 SNAKE_CASE로 표기하면 어떨까요?
이름으로 구분자 패턴임을 명확히 나타내면 좋을 것 같아요. 이름에 separator를 붙이면 어떨까요?
There was a problem hiding this comment.
앗 그러네요! 다음부터는 저런 상수는 표기법 바꿔서 해야겠네요. 이름만으로 파악하기 어려워서 SEPARATOR_PATTERN 같은 이름으로 바꾸는게 좋아보이네요!!
| export function getExpression(input) { | ||
| if (hasCustomSeparator(input)) { | ||
| return input.replace(pattern, ""); | ||
| } else { | ||
| return input; | ||
| } | ||
| } |
There was a problem hiding this comment.
[제안]
첨에 expression이 뭐지 싶어서 살짝 헷갈렸어요.
inputWithoutSeparatorPattern 같은.. 조금 더 명시적인 이름으로 하면 어떨까요?
아니면 expression 이름 그대로 가져가고 주석을 달아줘도 좋을 것 같네요.
There was a problem hiding this comment.
사실 저도 expression이라는 이름이 살짝 애매하다고 생각했는데, inputWithoutSeparatorPattern라고 쓰면 좀 길어지더라도 훨씬 명확해지네요! 이번 주차부터 JSDoc에 대해서 조금 공부해서 그거와 함께 주석을 추가하는 것도 좋은 방법일거 같습니다
| function isLegalNumber(number) { | ||
| if (number < 0) throw new Error("[ERROR] 양수를 입력해주세요."); | ||
| if (Number.isNaN(number)) | ||
| throw new Error("[ERROR] 올바른 숫자를 입력해주세요."); | ||
| } | ||
|
|
||
| function isLegelSeparator(separators) { | ||
| if (separators.length < 1) | ||
| throw new Error("[ERROR] 구분자를 올바르게 입력해주세요."); | ||
| if (/.*\d.*/.test(separators)) | ||
| throw new Error("[ERROR] 숫자를 구분자로 이용할 수 없어요."); | ||
| } |
There was a problem hiding this comment.
[제안]
함수 이름 앞에 is가 붙어서 boolean을 반환할 거라고 예상했는데 그게 아니어서 살짝 헷갈렸어요.
에러를 던지는 거면 validate같은 이름을 붙이면 어떨까요?
There was a problem hiding this comment.
앗 저도 is라고 적은게 맞는 표현인지 몰랐는데 validate라는 좋은 단어가 있네요!! 2주차부터 바로 반영해보겠습니다!
There was a problem hiding this comment.
좋은 제안 많이 해주셔서 감사합니다 😊 큰 도움되었습니다!!
3 participants
No description provided.