feat: 숫자를 순 우리말 수사로 변환하거나 수 관형사로 변환하는 함수를 추가 #201
Conversation
🦋 Changeset detectedLatest commit: 3d35964 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 95.18% 95.34% +0.15%
==========================================
Files 17 18 +1
Lines 291 322 +31
Branches 67 77 +10
==========================================
+ Hits 277 307 +30
- Misses 13 14 +1
Partials 1 1 |
src/susa.ts
Outdated
| let result = ''; | ||
|
|
||
| if (tens > 0 && hasProperty(SUSA_MAP, tens)) { | ||
| result += SUSA_MAP[tens]; | ||
| } | ||
| if (ones > 0 && hasProperty(SUSA_MAP, ones)) { | ||
| result += SUSA_MAP[ones]; | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
변할 수 있는 변수 선언인 let보다는 최대한 const를 사용해서, 사이드이펙트를 줄여보는것은 어떤가요?
아래와 같은 방법이 생각나는데, 더 좋은 방법이 있다면 그것도 좋아요!
| let result = ''; | |
| if (tens > 0 && hasProperty(SUSA_MAP, tens)) { | |
| result += SUSA_MAP[tens]; | |
| } | |
| if (ones > 0 && hasProperty(SUSA_MAP, ones)) { | |
| result += SUSA_MAP[ones]; | |
| } | |
| return result; | |
| const tensWord = tens > 0 && hasProperty(SUSA_MAP, tens) ? SUSA_MAP[tens] : ''; | |
| const onesWord = ones > 0 && hasProperty(SUSA_MAP, ones) ? SUSA_MAP[ones] : ''; | |
| return `${tensWord}${onesWord}`; |
There was a problem hiding this comment.
개선 의견 감사드립니다.
저에게 보기 더 좋아보이네요! 🙂
다만, 0보다 큰 조건은 삭제해도 될 것 같다는 의견입니다.
(아마 코드 초기에 필요했던 조건이 남아있던 것 같아요)
하여, 다음과 같이 수정하면 어떤가요?
const tensWord = hasProperty(SUSA_MAP, tens) ? SUSA_MAP[tens] : '';
const onesWord = hasProperty(SUSA_MAP, ones) ? SUSA_MAP[ones] : '';
return `${tensWord}${onesWord}`;There was a problem hiding this comment.
da227e8 에 반영했습니다.
| // 변환할 숫자 | ||
| num: number, | ||
| // 수 관형사를 사용할지 여부 | ||
| classifier?: boolean |
There was a problem hiding this comment.
요거 인터페이스를 아래와같이 해서 확장성을 가져갈 수도 있을 것 같은데, boolean을 그대로 받도록 하신 이유가 있으신가요?
저는 둘 다 좋은데, 이 기회에 es-hangul에 컨벤션(?) 을 잡아갈수도 있을 것 같아서요!
options : {
classifier?:boolean
}There was a problem hiding this comment.
저도 그 고민을 해보았지만, 확장할 여지가 없는 코어(?)에 가까운 함수라는 생각을 했어요. 제가 고민했던 확장 기능들은 다음과 같습니다.
십자리와 일자리 사이에 공백을 넣는 hasSpace 옵션
맞춤법에 어긋난 표현입니다. 수사나 수 관형사의 경우 붙여쓰는 것이 올바른 방법이라고 하더라구요.
뒤에 붙을 suffix: string를 받아서 수사와 수 관형사를 적절하게 붙여주는 옵션
함수를 사용하는 곳에서 충분히 처리할 수 있다고 판단했어요.
그래서 간단하게 boolean을 받도록 인터페이스를 구성했어요.
컨벤션(?) 관련해서 es-hangul을 쭉 살펴보았는데 옵셔널하게 파라미터를 받는 함수가 susa뿐 이군요!
수사와 수 관형사 함수를 분리해야하나 고민이 되기도 하네요. 🤔
정리하자면 제 의견은 'options로 확장성을 가져갈 수도 있겠지만, susa에서 확장될 여지가 있는 기능들은 모두 함수를 사용하는 곳에서 충분히 대처가 가능하다!' 입니다.
There was a problem hiding this comment.
컨벤션(?) 이야기가 나와서 코드를 살펴보다보니 규칙을 정하면 좋을 법한 부분들이 많은 것 같아요.
- 문자열을 받는 파라미터의 명칭이 어떤 곳에서는
str다른 곳에서는word,words(배열이 아님에도)으로 쓰이고 있다. jsdoc형태로 함수를 설명해 주는 곳이 있고,.md에서만 함수에 대한 설명을 볼 수 있는 곳이 있다.
등이 기여 시점에 기여자에게 안내되어 더 통일성있는 코드가 추가되면 좋겠네요. 🙂
BO-LIKE-CHICKEN
force-pushed
the
feature/susa
branch
from
767731b
to
1404a6f
Compare
|
feat: 정수가 아닌 경우에 대한 예외처리를 추가하고 분기를 탈 확률이 가장 낮은 조건을 뒤로 배치 에 정수가 아닌 값에 대한 예외처리도 추가하여 의견 주신 내용을 반영했습니다. 🙏🏻 |
|
안녕하세요, 이 부분에 대해서 제안 드려보고 싶은 점과 의문인 점이 있었어요 100 이상은 왜 안 되지?우선 docs 를 제대로 안 읽은 사람은 스크린샷의 정보만 읽을 것 같아서 JSDoc 이 추가되면 좋을 것 같구요 (타입으로 100 이하를 강제할 수는 없으니) 99 -> 아흔아홉 가 같은 맥락이라고 생각이 들었어요. 제가 잘 모르는 한글 도메인 맥락의 내용이 있다면 깨우쳐주시면 감사하겠습니다 확장가능의 여부라기 보다 인터페이스에서 상상이 안 되는 결과 값이에요
라고 답변을 해주셨는데 저는 아래와 같은 인터페이스를 봤을 때 susa(1, true) // ? false 주면 안 바꿔주나?수사 1 트루 라고 읽거든요, 어떤 결과 값이 나올지 doc 을 무조건 봐야 하는 인터페이스가 개선되었으면 하는데 어떻게 생각하시는지 궁금합니다. |
|
안녕하세요 @minsoo-web 님. 좋은 고민거리를 함께 나눠주셔서 감사해요! 먼저, 제 생각을 말씀드리기전에 의문이 드시는 부분을 다시금 정리해 보았어요. 의도를 잘못 파악했다면 말씀부탁드려요 🙏🏻
100 이상은
|
Overview
#169 에서 제안드린 함수를 추가합니다.
숫자를 순 우리말 수사로 변환하거나 수 관형사로 변환합니다. 주어진 숫자가 0보다 크고 100 이하일 때 유효합니다.
PR Checklist